Bug 36377 – new-run-webkit-tests doesn't detect wedged tests.

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

[Michael Moss]

Layout test framework doesn't detect wedged tests.

[Michael Moss]

Created attachment 51170 [details]
Patch

[Ojan Vafai]

Comment on attachment 51170 [details]
Patch

> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Michael Moss  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add layout_tests handling for unresponsive (but not dead) tests.

It's not clear how unresponsive and dead tests are different. Can you add a bit more description explaining that sometimes the the test_shell_thread itself gets hung when running a test? This is to clarify as opposed to a test that just never finishes or that hangs the TestShell/DRT process, both of which are already handled.

Code changes look good.

[Dirk Pranke]

Comment on attachment 51170 [details]
Patch

I'm not wild about this patch, simply because it checks for a wedged thread at (potentially) the wrong layer.

If you compare the chromium and mac implementations of driver.run_test, you'll see that the mac version actually implements non-blocking reads from DumpRenderTree, so that (in theory) it shouldn't ever wedge up. If we're seeing test_shell wedge up, I would probably be happier to change the Chromium implementation to do the same thing. We could probably refactor the non-blocking code into common logic shared in port/base.py.

In particular, this patch won't fix a wedged run if you run with --num-test-shells=1 (which runs the whole thing in a single thread).

On the other hand, we could decide that it's safer and more generic to do this at the thread-management layer (that way we can handle a wedged python thread regardless of what it's doing). But I would prefer to be clear about this rather than potentially be working around the same underlying problem two different ways.

[Michael Moss]

Created attachment 51178 [details]
Patch

[Michael Moss]

(In reply to comment #3)
> (From update of attachment 51170 [details])
> I'm not wild about this patch, simply because it checks for a wedged thread at
> (potentially) the wrong layer.
> 
> If you compare the chromium and mac implementations of driver.run_test, you'll
> see that the mac version actually implements non-blocking reads from
> DumpRenderTree, so that (in theory) it shouldn't ever wedge up. If we're seeing
> test_shell wedge up, I would probably be happier to change the Chromium
> implementation to do the same thing. We could probably refactor the
> non-blocking code into common logic shared in port/base.py.

Does O_NONBLOCK work on Windows? chromium.py is used on mac/linux/win.

> In particular, this patch won't fix a wedged run if you run with
> --num-test-shells=1 (which runs the whole thing in a single thread).

Yeah, I missed that usage.

> On the other hand, we could decide that it's safer and more generic to do this
> at the thread-management layer (that way we can handle a wedged python thread
> regardless of what it's doing). But I would prefer to be clear about this
> rather than potentially be working around the same underlying problem two
> different ways.

If O_NONBLOCK doesn't work, then this would still have the issue with --num-test-shells=1, right? Or is that not a big deal since --num-test-shells=1 is more of a debugging mode, and at least the buildbots would be able to recover better?

[Dirk Pranke]

(In reply to comment #5)
> Does O_NONBLOCK work on Windows? chromium.py is used on mac/linux/win.

Ooo, good question. It looks like the interweb is mixed in opinions on this; select() works on sockets, but I don't know if Windows pipes to a subprocess are considered sockets, and I don't know if fcntl will work but I'm not optimistic.

So, it may be that your approach is best and we'll have to expect num-test-shells=1 to potentially wedge (and maybe I can rip the non-blocking code out of the mac port).

We should at least check to make sure that you didn't do anything that causes num-test-shells=1 to outright break in the normal (non-wedging) case.

There's a PEP out for better non-blocking communication, but I'm not gonna hold my breath for that.

[Michael Moss]

> We should at least check to make sure that you didn't do anything that causes
> num-test-shells=1 to outright break in the normal (non-wedging) case.

unwedge() is only called in the _run_tests() threads loop, which num-test-shells=1 completely bypasses.

[Michael Moss]

(In reply to comment #7)
> > We should at least check to make sure that you didn't do anything that causes
> > num-test-shells=1 to outright break in the normal (non-wedging) case.
> 
> unwedge() is only called in the _run_tests() threads loop, which
> num-test-shells=1 completely bypasses.

Err, I mispoke. It doesn't bypass, but it finishes before it hits the loop, since all the processing happens in _instantiate_test_shell_threads().

[Michael Moss]

Created attachment 51324 [details]
Patch

[Dirk Pranke]

Comment on attachment 51324 [details]
Patch

it would be good if we could change the test results from CRASH to TIMEOUT if we had to kill the test shell to unwedge it, but I don't think you need to do that as a part of this patch. Please file a separate bug and I'll take a look at that if you don't want to.

Otherwise, this looks good to me.

[Michael Moss]

(In reply to comment #10)

Actually, I've been testing this on a Mac bot, and it's still not working quite right. unwedge() is triggering as expected, but it seems that some processes are so horribly wedged that 'kill' isn't always working, so run_webkit_tests can still hang until it's killed by the bot. I'm going to debug this some more and send another patch.

[Eric Seidel (no email)]

Comment on attachment 51324 [details]
Patch

r- based on Michael's comments.

[Dirk Pranke]

closing ... the latest version of the code does detect wedges.