Fixup timeouts #20321
Merged
Fixup timeouts #20321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
added
html
infra
wptrunner
wpt-pr-bot
requested review from
annevk,
domenic,
foolip,
gsnedders,
jdm,
jugglinmike,
tkent-google,
zcorpan and
zqzhang
LukeZielinski
approved these changes
Nov 20, 2019
Hexcles
approved these changes
Nov 20, 2019
| executor = threading.Thread(target=self.run_func) | ||
| executor.start() | ||
|
|
||
| flag = self.result_flag.wait(self.timeout + 2 * self.extra_timeout) |
There was a problem hiding this comment.
Nit: perhaps call this finished or something instead of the generic flag.
There was a problem hiding this comment.
Also maybe worth explaining here why we have a multiplier of 2 (I assume it's because the inner TestExecutor uses single extra_timeout).
| @@ -559,7 +561,19 @@ def run_test(self): | |||
| self.logger.info("Run %d/%d" % (self.run_count, self.rerun)) | |||
| self.send_message("reset") | |||
| self.run_count += 1 | |||
| wait_timeout = (self.state.test.timeout * self.executor_kwargs['timeout_multiplier'] + | |||
| 3 * self.executor_cls.extra_timeout) | |||
There was a problem hiding this comment.
Maybe worth documenting here why the multiplier is 3.
| test = self.state.test | ||
| self.test_ended(test, | ||
| (test.result_cls("EXTERNAL-TIMEOUT", | ||
| "Executor hit external timeout " |
There was a problem hiding this comment.
Shall we have a different message here to differentiate this from the EXTERNAL-TIMEOUT from executors? Maybe "TestRunner hit external timeout"?
| timer = threading.Timer(wait_timeout, self._timeout) | ||
| timer.start() | ||
|
|
||
| self._run() |
There was a problem hiding this comment.
I guess this is the root cause? We are stuck here forever even though the timer thread has set the flag...
|
There's a few unused And please take the Chromium export commit out before landing this PR. Thanks for the quick (and proper!) fix, James! |
jgraham
force-pushed
the
fixup_timeouts
branch
2 times, most recently
from
25b2843
to
1521212
Compare
Most executors were implementing very common variations on "run a function for up to some timeout and then mark the result as EXTERNAL-TIMEOUT". But the logic for these weren't shared and so fixes weren't shared between implementations. Move these into a common subclass such that the implementations can be shared, and adopt the logic from the WebDriverExecutor which runs the function in a thread and waits a given amount of time for the flag to be set. In contrast to the MarionetteExecutor implementation this actually works in the case where there's a browser hang.
In the case where the test just doesn't complete after the expected amount of time (plus some slack), forcibly end the test in the parent process. This eventually causes the child process to be restarted.
jgraham
force-pushed
the
fixup_timeouts
branch
from
1521212
to
78fc8a5
Compare
Hexcles
added a commit
that referenced
this pull request
Dec 6, 2019
After #20321, we now sometimes see an error from mozlog when the new external timeout is triggered: "ERROR test_end for ... logged while not in progress" A theory is that we call test_ended from a different thread which might not have all the correct states. This attempted fix switches to sending a test_ended message to the queue instead, similar to what TestRunner does normally.
Hexcles
added a commit
that referenced
this pull request
Dec 10, 2019
After #20321, we now sometimes see an error from mozlog when the new external timeout is triggered: "ERROR test_end for ... logged while not in progress" A theory is that we call test_ended from a different thread which might not have all the correct states. This attempted fix switches to sending a test_ended message to the queue instead, similar to what TestRunner does normally. In doing so, we also need to treat duplicate test_ended messages as non-fatal errors.
Hexcles
added a commit
that referenced
this pull request
Dec 10, 2019
After #20321, we now sometimes see an error from mozlog when the new external timeout is triggered: "ERROR test_end for ... logged while not in progress" A theory is that we call test_ended from a different thread which might not have all the correct states. This attempted fix switches to sending a test_ended message to the queue instead, similar to what TestRunner does normally. In doing so, we also need to treat duplicate test_ended messages as non-fatal errors. Hopefully this fixes #20607 .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.