-
Notifications
You must be signed in to change notification settings - Fork 184
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
refactor(iroh-sync): move replica event channels to iroh_sync, track open/close state of replicas #1640
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
Frando
force-pushed
the
sync-no-deadlocks-open-close
branch
from
October 16, 2023 11:13
4b0d97f
to
6091768
Compare
Frando
force-pushed
the
sync-no-deadlocks-open-close
branch
from
October 16, 2023 11:41
46b9625
to
b8eb3f8
Compare
Frando
force-pushed
the
sync-no-deadlocks-open-close
branch
from
October 16, 2023 11:57
978467e
to
9330546
Compare
3 tasks
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.
Description
This PR contains changes on top of #1612. Made a separate PR to ease reviewing. I'd merge this into #1612 after a review here, and then hopefully merge #1612 into main right after.
Has the following changes:
Change the subscription model on
Replica
s to be aVec<flume::Sender<Event>>
. This now allows to directly subscribe to replica events from multiple places. This allows us to have subscription to replica events from the client not go through any actor. The replica events are merged together with the events from thesync_engine::live
actor, and that's it. Should improve latency and ease work on the actors. Because we merge the channels for replica events and live actor events for client subscriptions, this means, that closing a replica from the client does not end the subscription stream, because the subscription on the live actor will remain active. To end the subscription, the client would need to drop the receiver. I think this is fine for now. What we might want to do instead is to split the subscription from the client to two methods and channels,subscribe_insert_events
andsubscribe_sync_events
or so, then we could close them separately. However the one for sync events would still stay open indefinitely, because you wouldn't want to drop and recreate, I think, in case you dojoin
/leave
/join
. Or do we?Track the open state of Replicas in the
iroh_sync::actor
. I opted for a simple implementation to start: The actor counts calls toopen
and decrements on calls toclose
and closes the replica once the count reaches zero. This works fine, however for replicas opened from the RPC client, because there's no async drop, I spawn a tokio task in drop to send the close call to the node. This means that if a client is force-killed, the replica would remain open indefinitely. It would be better to solve this cleaner - the only idea I had so far was to give out something likeReplicaDescriptor
s onopen
and then send require regular keep-alive calls from the RPC client. I think it's fine to defer this change to a followup (which will be straightforward with the architecture in place now) because the impl in this PR is already quite an improvement over the state inmain
or refactor(iroh-sync): Add actor to iroh-sync, remove deadlocks #1612, where we don't ever close replicas.event subscriptions for replica insert events are now handled directly in the
iroh_sync::actor
, which is much nicer IMO. For the RPC subscription this event stream is merged with a subscription for sync events from theiroh::sync_engine::live
actor.Notes & open questions
included in description..
One more thought: Now that the
Replica
s are usually only used in theiroh_sync::actor::Actor
, which is a single loop with&mut
access, we could think about removing theClone
impl and allo locks from theReplica
. If you wanted to useReplica
s outside of the actor, you'd have to lock them yourself. Didn't think it through fully, but possibly worthwhile to investigate.Change checklist