new-run-webkit-tests: doesn't wait for children if it gets a ctrl-c
Created attachment 92364 [details] Patch
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?
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.
Comment on attachment 92364 [details] Patch Sounds like dirk is gonna take another pass.
Created attachment 95374 [details] Patch
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.
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?
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.
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?
(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.
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.
Comment on attachment 95374 [details] Patch Clearing flags on attachment: 95374 Committed r87757: <http://trac.webkit.org/changeset/87757>
All reviewed patches have been landed. Closing bug.