This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Split NetworkBridge and break cycles with Unbounded #2736
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
Co-authored-by: Peter Goodspeed-Niklaus <[email protected]>
1. we don't provide good names 2. these names are never used anywhere
We have channel size metrics that serve the same purpose better now and the implementation of message timing was pretty ugly.
rphmeier
added
B0-silent
github-actions
bot
added
the
A3-in_progress
rphmeier
added
A0-please_review
ordian
reviewed
Mar 28, 2021
Comment on lines
+617
to
+625
| /// THIS IS A HACK. We need to ensure we never hold the mutex across a `.await` boundary | ||
| /// and `parking_lot` currently does not provide `Send`, which helps us enforce that. | ||
| /// If this breaks, we need to find another way to protect ourselves. | ||
| /// | ||
| /// ```compile_fail | ||
| /// #use parking_lot::MutexGuard; | ||
| /// #fn is_send<T: Send>(); | ||
| /// #is_send::<parking_lot::MutexGuard<'static, ()>(); | ||
| /// ``` |
There was a problem hiding this comment.
haven't look at the actual issue, but if we need an async-aware Mutex, there is https://docs.rs/futures/0.3.13/futures/lock/struct.Mutex.html?
There was a problem hiding this comment.
@ordian I think parking_lot::Mutex is fine here. To avoid deadlock the enforcement that we never use the mutex across a .await is helpful. It means we will never be waiting to unpark the thread for very long, and we never run into a situation where:
- Side A is acquiring the mutex
- Side B is holding the mutex and waiting for side A to drain a buffer
Co-authored-by: Andronik Ordian <[email protected]>
rphmeier
changed the title
SubsystemSender and other subsystems to SubsystemSender
eskimor
approved these changes
Mar 28, 2021
rphmeier
added a commit
that referenced
this pull request
Mar 28, 2021
* overseer: pass messages directly between subsystems * test that message is held on to * Update node/overseer/src/lib.rs Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> * give every subsystem an unbounded sender too * remove metered_channel::name 1. we don't provide good names 2. these names are never used anywhere * unused mut * remove unnecessary &mut * subsystem unbounded_send * remove unused MaybeTimer We have channel size metrics that serve the same purpose better now and the implementation of message timing was pretty ugly. * remove comment * split up senders and receivers * update metrics * fix tests * fix test subsystem context * use SubsystemSender in jobs system now * refactor of awful jobs code * expose public `run` on JobSubsystem * update candidate backing to new jobs & use unbounded * bitfield signing * candidate-selection * provisioner * approval voting: send unbounded for assignment/approvals * async not needed * begin bridge split * split up network tasks into background worker * port over network bridge * Update node/network/bridge/src/lib.rs Co-authored-by: Andronik Ordian <[email protected]> * rename ValidationWorkerNotifications Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> Co-authored-by: Andronik Ordian <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Based on #2227
This also includes changes to the jobs system and subsystems making use of it to expose the
SubsystemSenderto each job.Also:
send_unbounded_messagein candidate backing to send statements to statement distributionsend_unbounded_messagein approval voting to sent messages to approval distribution