#10314 Better worker-side error handling in disttrial #11644
Merged
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
|
please review |
adiroiban
reviewed
Sep 9, 2022
There was a problem hiding this comment.
I did a very quick review.
I am leaving the command as I don't know when I will have time to do a full review.
If no other developer will have time to review this in the next few days, I think that this should be merge.
I don't think is ok to block a merge for a PR that has tests for the new code, is well documented and the tests are green.
exarkun
force-pushed
the
10314.disttrial-error-handling
branch
from
8d0411e
to
5cc7599
Compare
This avoids a protocol error when the string representations exceed the length limits imposed by the AMP protocol disttrial uses.
exarkun
force-pushed
the
10314.disttrial-error-handling
branch
from
5cc7599
to
5811136
Compare
adiroiban
reviewed
Sep 14, 2022
It's still on a class in a private package so this doesn't make it public to users outside of Twisted (or even outside of disttrial). It does make it public to `WorkerProtocol` which needs to use it.
adiroiban
reviewed
Sep 15, 2022
adiroiban
approved these changes
Sep 15, 2022
All reporting results must have happened by the time `suite.run` returns so there's no reason to process them inside the context manager. And processing them in the context manager means that if any code somehow starts a _new_ reporting operation the list we passed to DeferredList will be mutated (and presumably then ignore). Ending the context manager turns that case into a hard error, instead.
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.
Scope and purpose
Fixes #10314
This adds error handling (any, at all!) to the "workerreporter" that manages communication of test results from the disttrial worker processes back to the parent process so that such errors won't cause the whole trial process to exit without a result report.
It also changes the network protocol between workers and parent to stream error text so that text longer than the AMP limit of 64k makes it back to the parent without error.