Bug 51572 – new-run-webkit-tests: shut up and exit when all the threads are wedged

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

shut up and exit when all the threads are wedged

[Dirk Pranke]

Created attachment 77380 [details]
Patch

[Mihai Parparita]

Comment on attachment 77380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77380&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittest.py:148
> +    def update(self):

There's a MockTestRunner in this file, can you use that?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 77380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77380&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittest.py:148
> > +    def update(self):
> 
> There's a MockTestRunner in this file, can you use that?

Ah, good point. That's actually in a different class, but there's no compelling need to have two separate classes here, so I'll combine them.

[Dirk Pranke]

Created attachment 77397 [details]
use MockTestRunner()

[Dirk Pranke]

Adding some more reviewers ... one of you want to review and/or rubber-stamp this? It's very small and should help the bots out (and anyone who runs the layout tests manually ...)

[Adam Barth]

Comment on attachment 77397 [details]
use MockTestRunner()

I love the name of this bug.

[Eric Seidel (no email)]

Comment on attachment 77397 [details]
use MockTestRunner()

I'm not sure I understand what this does.

[Eric Seidel (no email)]

Comment on attachment 77397 [details]
use MockTestRunner()

I think the cq will try to land this if it has a cq+, even if it still has r?  (and will fail).  Setting to cq? to prevent that.

[Kenneth Russell]

Comment on attachment 77397 [details]
use MockTestRunner()

I gather that this causes us to log only once per wedged thread and to abort if all threads get wedged. Two questions: (1) if this happens, is bailing out the right thing to do? Or should some other action (like spawning more threads) be taken? (2) Will the warning and early exit be properly handled as a failure?

[Dirk Pranke]

(In reply to comment #9)
> (From update of attachment 77397 [details])
> I gather that this causes us to log only once per wedged thread and to abort if all threads get wedged. 

Ken's analysis is right. Right now if we get wedged we will log very frequently and run for a very long time before eventually either getting killed by the bot or we make enough progress to exit (although I'm not sure why the bot doesn't always get stuck). This causes things to fail fast.

 > Two questions: (1) if this happens, is bailing out the right thing to do? Or should some other action (like spawning more threads) be taken?

If threads get wedged, then the process will never exit cleanly and the test run will be considered a failure. In addition, one or more tests will never complete. Given the way the code currently handles sharding the tests, large numbers of tests might not get to run.

This is by no means a real fix, it is just intended to make the behavior less bad. The fix is the other set of patches I've been working on.

>  (2) Will the warning and early exit be properly handled as a failure?

Yes.

[Kenneth Russell]

Comment on attachment 77397 [details]
use MockTestRunner()

Even if this isn't the final fix it's still a major improvement over the current situation. r=me

[WebKit Commit Bot]

Comment on attachment 77397 [details]
use MockTestRunner()

Clearing flags on attachment: 77397

Committed r74632: <http://trac.webkit.org/changeset/74632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Sorry but I had to rollout this patch. The unittest makes test-webkitpy hang forever on the GTK bots.

See also bug 51600

[Dirk Pranke]

Sigh. Sorry for the brokenness - I have no idea what's different about your config that is causing it to hang, but Python will wait for every spawned thread to exit before exiting the process, so that's probably what's going on here.

What version of Linux runs on the GTK bots?

[Eric Seidel (no email)]

I just saw a commit-queue node hang in the python tests 9 hours ago, so unless this was re-landed, this was not the only cause.

[Philippe Normand]

(In reply to comment #15)
> Sigh. Sorry for the brokenness - I have no idea what's different about your config that is causing it to hang, but Python will wait for every spawned thread to exit before exiting the process, so that's probably what's going on here.
> 
> What version of Linux runs on the GTK bots?

I could reproduce the issue on my laptop running Debian Sid. The GTK Debug bots run on Debian Sid as well, IIRC.

[Dirk Pranke]

Hmm. I don't know that I have an easy way to get an instance of Debian Sid to test this on. After I poke into this some more, and if it works for me on the Ubuntu instances I have, I may need your help to figure this one out.

[Dirk Pranke]

Created attachment 81020 [details]
update w/ fix in r76073, try again

[Kenneth Russell]

Comment on attachment 81020 [details]
update w/ fix in r76073, try again

OK.

[Dirk Pranke]

Created attachment 81173 [details]
remove duplicated code

[Dirk Pranke]

Committed r77586: <http://trac.webkit.org/changeset/77586>

[Dirk Pranke]

Committed r77595: <http://trac.webkit.org/changeset/77595>

[Dirk Pranke]

*** Bug 46833 has been marked as a duplicate of this bug. ***

[Dirk Pranke]

*** Bug 46320 has been marked as a duplicate of this bug. ***