Bug 60241

Summary: new-run-webkit-tests: doesn't wait for children if it gets a ctrl-c
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Dirk Pranke
Reported 2011-05-04 19:47:30 PDT
new-run-webkit-tests: doesn't wait for children if it gets a ctrl-c
Attachments
Patch (1.58 KB, patch)
2011-05-04 19:47 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
Patch (1.51 KB, patch)
2011-05-30 15:05 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dirk Pranke
Comment 1 2011-05-04 19:47:51 PDT
Created attachment 92364 [details] Patch
Tony Chang
Comment 2 2011-05-06 15:25:55 PDT
Comment on attachment 92364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92364&action=review > Tools/Scripts/new-run-webkit-tests:59 > + finally: Do we need to catch other exceptions? E.g., if proc.wait() throws another error (e.g., an IO or OS error), it looks like we'll just silently ignore the error if we make it to sys.exit(proc.returncode). Maybe that's not possible?
Dirk Pranke
Comment 3 2011-05-06 15:41:54 PDT
Comment on attachment 92364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92364&action=review >> Tools/Scripts/new-run-webkit-tests:59 >> + finally: > > Do we need to catch other exceptions? E.g., if proc.wait() throws another error (e.g., an IO or OS error), it looks like we'll just silently ignore the error if we make it to sys.exit(proc.returncode). Maybe that's not possible? Hm. Good point. I should rework this.
Eric Seidel (no email)
Comment 4 2011-05-11 20:16:47 PDT
Comment on attachment 92364 [details] Patch Sounds like dirk is gonna take another pass.
Dirk Pranke
Comment 5 2011-05-30 15:05:22 PDT
Created attachment 95374 [details] Patch
Dirk Pranke
Comment 6 2011-05-30 15:08:10 PDT
Comment on attachment 95374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95374&action=review Okay, reworked. Can haz another review, plz? > Tools/Scripts/new-run-webkit-tests:-57 > - sys.exit(signal.SIGINT + 128) Note that we don't need or want this line, because the same logic exists the subprocess, and omitting this will help catch cases where the subprocess starts doing the wrong thing.
Tony Chang
Comment 7 2011-05-31 12:06:47 PDT
Comment on attachment 95374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95374&action=review > Tools/Scripts/new-run-webkit-tests:59 > + proc.wait() Is it possible for this to hang completely?
Dirk Pranke
Comment 8 2011-05-31 14:34:14 PDT
Comment on attachment 95374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95374&action=review >> Tools/Scripts/new-run-webkit-tests:59 >> + proc.wait() > > Is it possible for this to hang completely? Good question (since that's why I put the fixme in). I haven't seen in in practice, and in theory, I wouldn't think so (since the subprocess also got a Keyboard interrupt and should be exiting), but I can't definitively say that it's not possible.
Tony Chang
Comment 9 2011-05-31 14:37:23 PDT
Comment on attachment 95374 [details] Patch Ok, let's try it. I guess worst case scenario, the user will press ctrl+c again and both the subprocess and the main process will raise a KeyboardInterrupt exception, right?
Dirk Pranke
Comment 10 2011-05-31 14:39:45 PDT
(In reply to comment #9) > (From update of attachment 95374 [details]) > Ok, let's try it. I guess worst case scenario, the user will press ctrl+c again and both the subprocess and the main process will raise a KeyboardInterrupt exception, right? Yup.
WebKit Commit Bot
Comment 11 2011-05-31 15:25:37 PDT
The commit-queue encountered the following flaky tests while processing attachment 95374 [details]: editing/pasteboard/copy-standalone-image.html bug 61813 (authors: andrew@webkit.org and mitz@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 12 2011-05-31 15:27:07 PDT
Comment on attachment 95374 [details] Patch Clearing flags on attachment: 95374 Committed r87757: <http://trac.webkit.org/changeset/87757>
WebKit Commit Bot
Comment 13 2011-05-31 15:27:13 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.