Bug 60241 – new-run-webkit-tests: doesn't wait for children if it gets a ctrl-c

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: doesn't wait for children if it gets a ctrl-c

[Dirk Pranke]

Created attachment 92364 [details]
Patch

[Tony Chang]

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 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 on attachment 92364 [details]
Patch

Sounds like dirk is gonna take another pass.

[Dirk Pranke]

Created attachment 95374 [details]
Patch

[Dirk Pranke]

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

(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]

The commit-queue encountered the following flaky tests while processing attachment 95374 [details]:

editing/pasteboard/copy-standalone-image.html bug 61813 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95374 [details]
Patch

Clearing flags on attachment: 95374

Committed r87757: <http://trac.webkit.org/changeset/87757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.