Bug 226238

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=226234

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

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	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
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.