Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shutdown: Select on listener on all channel sends #193

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Aug 1, 2024

Ran into a deadlock on shutdown when running with > 100 nodes. Cause was multiple send calls trying to pass events on to receiver channels that had already exited (because listener signaled that they should), example here.

This is difficult to hit with smaller networks, because most of the time everything just exits with the listener signal - it's only when we're in the middle of an operation in multiple tasks where we hit this.

This PR takes the approach of just selecting on sender/listener to exit on the sender side. An alternative approach would be to use receiver.close() and drain the channel. This is a little more boilerplate to update all of our receiver sites, so I went with the sender approach.

Tracing via tokio-console is also added behind a development flag, so that debugging issues like this is easier in the future (it has gorgeous dashboards 😻 ).

Related to #162.

@carlaKC carlaKC requested a review from enigbe August 1, 2024 13:20
@carlaKC carlaKC force-pushed the select-send branch 3 times, most recently from 11f313b to 61feaf8 Compare August 1, 2024 13:45
@carlaKC
Copy link
Contributor Author

carlaKC commented Aug 1, 2024

Looking into the build error, can't reproduce locally.

@sr-gi
Copy link
Member

sr-gi commented Aug 1, 2024

Looking into the build error, can't reproduce locally.

I think it may be a bug upstream with time. Updating the library may fix it.

ref: time-rs/time#693

@carlaKC
Copy link
Contributor Author

carlaKC commented Aug 9, 2024

Updating the library may fix it.

Indeed! Didn't break with my version of rustc - will rebsae this once #195 is in!

Copy link
Collaborator

@enigbe enigbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on this @carlaKC. Happy to approve these changes.

@carlaKC carlaKC merged commit d8b5971 into bitcoin-dev-project:main Aug 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants