RESOLVED FIXED 226238
[webkitcorey] Gracefully handle CNTRL-C in TaskPool 
https://bugs.webkit.org/show_bug.cgi?id=226238
Summary [webkitcorey] Gracefully handle CNTRL-C in TaskPool
Jonathan Bedard
Reported 2021-05-25 13:05:26 PDT
When a user CNTRL-C's a Python application, that signal is forwarded to all child processes. This works well, until it's time to tear down the queue between processes, when Python starts logging error messages about broken pipes. We want to suppress these error messages, but also allow the TaskPool to be re-used.
Attachments
Patch (5.25 KB, patch)
2021-05-25 13:37 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.31 KB, patch)
2021-05-26 09:36 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.33 KB, patch)
2021-05-27 15:48 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch for landing (6.65 KB, patch)
2021-05-27 18:03 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-05-25 13:05:46 PDT
<rdar://problem/78472148>
Jonathan Bedard
Comment 2 2021-05-25 13:37:03 PDT
Created attachment 429689 [details] Patch
Jonathan Bedard
Comment 3 2021-05-26 09:36:11 PDT
Created attachment 429765 [details] Patch
dewei_zhu
Comment 4 2021-05-27 15:29:53 PDT
Comment on attachment 429765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429765&action=review > Tools/ChangeLog:13 > + (TaskPool.__init__): Defer worker and queue construction to context manager. Could you tell me more about this deferral? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/task_pool.py:-335 > - from mock import patch Could you tell me more about this change? I think this was for making unit tests happy?
Jonathan Bedard
Comment 5 2021-05-27 15:37:23 PDT
Comment on attachment 429765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429765&action=review >> Tools/ChangeLog:13 >> + (TaskPool.__init__): Defer worker and queue construction to context manager. > > Could you tell me more about this deferral? The point of this deferral is so that we never have unusable but instantiated queues. Basically, if children processes receive a CNTRL-C, the queues in the parent process will end up with broken pipes. We then need to close, clean up and re-construct those queues. Given that the queues and workers aren't really valid until we start them in the first place, it seemed best to tie both the queue and the workers directly to the context manager. >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/task_pool.py:-335 >> - from mock import patch > > Could you tell me more about this change? I think this was for making unit tests happy? Oh, oops, yeah, I should put that back in. My IDE said it was unused, and I wasn't paying attention
Jonathan Bedard
Comment 6 2021-05-27 15:48:12 PDT
Created attachment 429945 [details] Patch
dewei_zhu
Comment 7 2021-05-27 16:41:56 PDT
Comment on attachment 429945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429945&action=review r=me > Tools/ChangeLog:13 > + (TaskPool.__init__): Defer worker and queue construction to context manager. Might be useful to mention why we want to do this.
Jonathan Bedard
Comment 8 2021-05-27 18:03:27 PDT
Created attachment 429969 [details] Patch for landing
EWS
Comment 9 2021-05-27 19:27:07 PDT
Committed r278187 (238230@main): <https://commits.webkit.org/238230@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429969 [details].
Note You need to log in before you can comment on or make changes to this bug.