Summary: | [webkitcorey] Gracefully handle CNTRL-C in TaskPool | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aakash_jain, dewei_zhu, slewis, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226234 | ||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2021-05-25 13:05:26 PDT
Created attachment 429689 [details]
Patch
Created attachment 429765 [details]
Patch
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? 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 Created attachment 429945 [details]
Patch
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. Created attachment 429969 [details]
Patch for landing
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]. |