1248056 - Marionette connection timeout when running mochitest with --valgrind

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew Halberstadt [:ahal]]

This is fallout from bug 1231784.

See:
https://public-artifacts.taskcluster.net/RDU46_RkTIqPn3ifDzr9qg/0/public/logs/live_backing.log

Apparently using --valgrind can make Gecko *really* slow to start up. My theory is that the default 60 second connection timeout in marionette isn't sufficient:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#1130

Ted points out that it may also be insufficient when running with certain debuggers attached. If this is the case, we'll need to bump that timeout when running with these types of tools.

Comment 1

[Julian Seward [:jseward]]

Attachment: demo patch (do not commit)

Just to confirm, per irc w/ ahal on Thursday (?), increasing the
marionette startup timeout from 60 seconds to 900 seconds fixed the
problem for me, when running on Valgrind.  As ahal suspected it would.

Comment 2

[Andrew Halberstadt [:ahal]]

Excellent. I'll add a hidden argument for controlling the marionette timeout. Then I'll automatically bump it up higher if --valgrind or --debugger is set.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1248056-1.diff

I found a second timeout that needs to be adjusted.  So here's a
revised reference patch that shows both changes.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1248056 - Increase marionette connection timeouts in mochitest when running with valgrind, r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/35113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35113/

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8719781 [details]
MozReview Request: Bug 1248056 - Increase marionette connection timeouts in mochitest when running with valgrind, r?chmanchester

https://reviewboard.mozilla.org/r/35113/#review31729

::: testing/mochitest/mochitest_options.py:565
(Diff revision 1)
> +        [["--marionette-port-timeout"],
> +         {"default": 60,
> +          "help": "Timeout while waiting for the marionette port to open.",
> +          "suppress": True,
> +          }],
> +        [["--marionette-socket-timeout"],
> +         {"default": 360.0,
> +          "help": "Timeout while waiting to receive a message from the marionette server.",
> +          "suppress": True,
> +          }],

Consider leaving these un-set when they're not needed so we don't have these defaults both here and in marionette.

::: testing/mochitest/runtests.py:2356
(Diff revision 1)
>              marionette_args = {
>                  'symbols_path': options.symbolsPath,
> +                'socket_timeout': options.marionette_socket_timeout,
> +                'port_timeout': options.marionette_port_timeout,

These args end up in the constructor for marionette, right? I don't see port_timeout as a valid keyword argument there.

Comment 6

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/35113/#review31729

> Consider leaving these un-set when they're not needed so we don't have these defaults both here and in marionette.

I was trying to avoid having to wrap all my marionette calls in if statements as passing in None to them overrides the default. But I'll fix this by making a change to marionette as well.

> These args end up in the constructor for marionette, right? I don't see port_timeout as a valid keyword argument there.

It gets popped back off before being passed to the constructor. I did it this way to avoid changing the signature of runApp (which meant futzing with b2g and android). But if you feel particularly strongly about it, I can pass it in via the runApp signature instead of piggy-backing off marionette_args.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8719781 [details]
MozReview Request: Bug 1248056 - Increase marionette connection timeouts in mochitest when running with valgrind, r?chmanchester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35113/diff/1-2/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf27afd3c5c6

Comment 9

[Peter Van der Beken [:peterv]]

Does it even make sense to have a timeout for an 'interactive' debugger? The debugger could just be waiting for the user to give the command to start the target, why would timing out make sense in that case?

Comment 10

[Andrew Halberstadt [:ahal]]

That's a good point. Marionette currently polls for the server every 100ms, and there isn't any way to disable the timeout completely (though that could be changed). Though one benefit of a timeout is to prevent this from accidentally getting run indefinitely in a screen session or similar.

But even so, with this patch developers have 15 minutes to start the target. And if they somehow get distracted and miss this timeout, it's just a matter of re-running the command. For those reasons this use case isn't terribly compelling to me. Though I'm open to further debate.

Comment 11

[Peter Van der Beken [:peterv]]

15 minutes certainly sounds better already. I've had multiple instances where I was looking up where to set a breakpoint only to have my run killed once I switched back to my terminal. I still don't think that that timeout makes sense, but I guess I'll just set a huge one using the --marionette-*-timeout arguments.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf27afd3c5c6

Comment 13

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8842acd6224

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Comment on attachment 8719781 [details]
> MozReview Request: Bug 1248056 - Increase marionette connection timeouts in
> mochitest when running with valgrind, r?chmanchester

The patch as landed here was not that ideal and doesn't correspond with the commit message as given for mozreview. It actually bumped up the default socket timeout for each and every Marionette based test harness. As result also tests running on opt builds had this timeout set, which increased our test execution times drastically in case of socket failures. I have seen this a lot lately due to all sort of e10s issues. 

Please keep in mind that I reverted the default value back to 60s on bug 1284457 to get rid of those long wait times on build and test machines. So if any other test harness or developers want to use a different timeout they will have to specify that via the command line option --socket-timeout.