Keep track of background peer tasks #3253
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
3 tasks
teor2345
added
A-diagnostics
|
I'm marking this as draft because the tests are failing, and it depends on another PR merging. |
teor2345
reviewed
Dec 17, 2021
There was a problem hiding this comment.
This looks like a good design, but I have some questions about type-safety and test coverage.
I'd like to merge the following PR before we merge this PR:
- to see coverage on this PR: Re-enable code coverage comments on PRs #3246
Do you think we should merge these test/fix PRs before the changes in this PR?
- test coverage: Add PeerSet readiness and request future cancel-safety tests #3252
peer-set related bug fixes: Fix task handling bugs, so peers are more likely to be available #3191
jvff
force-pushed
the
keep-track-of-peer-heartbeat-tasks
branch
from
33ba182 to
69b3933
Compare
jvff
force-pushed
the
use-mocked-client-handle-in-other-tests
branch
from
c174644 to
057d162
Compare
jvff
force-pushed
the
keep-track-of-peer-heartbeat-tasks
branch
from
69b3933 to
d40ed78
Compare
teor2345
reviewed
Dec 19, 2021
jvff
force-pushed
the
keep-track-of-peer-heartbeat-tasks
branch
from
d40ed78 to
52e22e2
Compare
Base automatically changed from
use-mocked-client-handle-in-other-tests
to
main
December 21, 2021 20:13
Move the code that's part of the heartbeat task into a separate helper function.
Keep it closer to where it's actually used, and make it easier to add new fields to `Client` for the connection and heartbeat tasks.
Prepare it to be able to check for panics or errors from the background tasks.
Spawn simple timeout tasks as mock connection and heartbeat tasks.
Building a `ClientTestHarness` requires a Tokio runtime to be set up, so the calls were moved into the `async` block.
Make the code reusable for both background tasks.
Periodically poll it to check if the task has unexpectedly stopped.
The client service should stop if the connection background task has exited, because then it's not able to receive any replies.
Wrap the background tasks in `Abortable`, so that they can be aborted through the `ClientTestHarness`.
Check that stopping the background connection task is something that the `Client` instance detects and handles correctly.
Check that stopping the background heartbeat task is something that the `Client` instance detects and handles correctly.
Will be used later to create background tasks that panic.
Use a mock background connection task that panics immediately, and check that the `Client` handles it gracefully.
Use a mock background heartbeat task that panics immediately, and check that the `Client` handles it gracefully.
The previously linked issue was a broad plan to improve Zebra's shutdown behavior, while the new issue is more specific, and can be scheduled sooner. Co-authored-by: teor <[email protected]>
jvff
force-pushed
the
keep-track-of-peer-heartbeat-tasks
branch
from
52e22e2 to
235219b
Compare
teor2345
approved these changes
Dec 21, 2021
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.
Motivation
When setting up a
Clientservice for a peer connection, some background tasks are spawned. These tasks canpanic, and if the spawned task'sJoinHandleisn't polled, Zebra might miss the error.This closes #3199, but currently depends on #3241 being merged first.
Solution
Store a
JoinHandlefor each task spawned for the peer connection inside theClienttype, and poll them insidepoll_ready.Review
@teor2345
Reviewer Checklist
Follow Up Work
JoinHandle::abortmethod instead?