Wait for metrics handling to finish even when there is an error #2798
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
codebien
previously approved these changes
Dec 5, 2022
olegbespalov
previously approved these changes
Dec 5, 2022
imiric
reviewed
Dec 5, 2022
na--
dismissed stale reviews from olegbespalov, codebien, and oleiade
via
cea7276
na--
force-pushed
the
fix-run-error-wait
branch
from
138246a
to
cea7276
Compare
imiric
previously approved these changes
Dec 5, 2022
Previous to this, setup() and teardown() exceptions or test.abort() calls would have made k6 exit prematurely, without waiting for the metrics processing to finish. This also caused a data race in integration tests and undefined behavior in general. The handleSummary() function was also called only when test.abort() was used, but not when there was an error in `setup()` and `teardown()`. The commit also adds and improves a bunch of integration tests, particularly when it comes to validating aborted tests and --linger.
na--
force-pushed
the
fix-run-error-wait
branch
from
cea7276
to
a8ff4a1
Compare
|
Weird 😕 force-pushed again, with no changes besides the commit message, and CI seems to have started fine this time 🤞 |
codebien
approved these changes
Dec 5, 2022
imiric
approved these changes
Dec 5, 2022
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.
Before these changes,
setup()andteardown()exceptions ortest.abort()calls would have made k6 exit prematurely, without waiting for the metrics processing to finish. This bug was probably a cause for some of the "timed out"k6 run --out cloudtests we have seen, e.g. if they had some sort an error insetup()orteardown(). It also caused a data race in the recent integration tests, this is how I caught it. So, as a nice bonus, this fix allowed me to remove one of theFIXMEs for unnecessary locking in there earlier than expected 🎉This is another thing that should be fixed by the new
Engine-less architecture (#1889). However, once again, the fix was easy enough to do even now, and the bug severe enough that it's probably worth it to include it in the upcoming k6 v0.42.0.I also added and expanded a bunch of integration tests, particularly when it comes to validating aborted tests and
--linger, and this PR is built on top of #2795, so hopefully its last minute merging won't cause any regressions 🤞 🙏 📿 .It seemed like the bug might have been introduced in k6 v0.36.0, via #2093 (the PR that added
test.abort()), but it was actually probably introduced way back in k6 v0.27.0, with #1390, or even older... 😞 Still, the added tests fortest.abort()and setup/teardown exceptions should provide some assurance now and in the future.