Bug 226238 - [webkitcorey] Gracefully handle CNTRL-C in TaskPool
Summary: [webkitcorey] Gracefully handle CNTRL-C in TaskPool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-25 13:05 PDT by Jonathan Bedard
Modified: 2021-05-27 19:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.25 KB, patch)
2021-05-25 13:37 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (5.31 KB, patch)
2021-05-26 09:36 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (5.33 KB, patch)
2021-05-27 15:48 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (6.65 KB, patch)
2021-05-27 18:03 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 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.
Comment 1 Radar WebKit Bug Importer 2021-05-25 13:05:46 PDT
<rdar://problem/78472148>
Comment 2 Jonathan Bedard 2021-05-25 13:37:03 PDT
Created attachment 429689 [details]
Patch
Comment 3 Jonathan Bedard 2021-05-26 09:36:11 PDT
Created attachment 429765 [details]
Patch
Comment 4 dewei_zhu 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?
Comment 5 Jonathan Bedard 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
Comment 6 Jonathan Bedard 2021-05-27 15:48:12 PDT
Created attachment 429945 [details]
Patch
Comment 7 dewei_zhu 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.
Comment 8 Jonathan Bedard 2021-05-27 18:03:27 PDT
Created attachment 429969 [details]
Patch for landing
Comment 9 EWS 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].