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

refactor(iroh-sync): move replica event channels to iroh_sync, track open/close state of replicas #1640

Merged
merged 6 commits into from
Oct 16, 2023

Conversation

Frando
Copy link
Member

@Frando Frando commented Oct 16, 2023

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 Replicas to be a Vec<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 the sync_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 and subscribe_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 do join / 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 to open and decrements on calls to close 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 like ReplicaDescriptors on open 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 in main 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 the iroh::sync_engine::live actor.

Notes & open questions

included in description..

One more thought: Now that the Replicas are usually only used in the iroh_sync::actor::Actor, which is a single loop with &mut access, we could think about removing the Clone impl and allo locks from the Replica. If you wanted to use Replicas outside of the actor, you'd have to lock them yourself. Didn't think it through fully, but possibly worthwhile to investigate.

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@Frando Frando force-pushed the sync-no-deadlocks-open-close branch from 4b0d97f to 6091768 Compare October 16, 2023 11:13
@Frando Frando force-pushed the sync-no-deadlocks-open-close branch from 46b9625 to b8eb3f8 Compare October 16, 2023 11:41
@Frando Frando force-pushed the sync-no-deadlocks-open-close branch from 978467e to 9330546 Compare October 16, 2023 11:57
@Frando Frando merged commit 051ca33 into sync-no-deadlocks Oct 16, 2023
@dignifiedquire dignifiedquire deleted the sync-no-deadlocks-open-close branch October 16, 2023 17:08
@b5 b5 added this to the v0.8.0 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants