feat(iroh-gossip)!: dispatch gossip events and updates by topic #2570

Frando · 2024-07-31T20:30:50Z

Description

This PR changes the main public iroh_gossip to keep track of client-side gossip subscriptions. The net::Gossip struct now keeps track of client-side subscribers per topic, which are made up of a pair of two streams/channels: from the client to the actor a stream of updates (outgoing messages) and from the actor to the client a stream of events (incoming messages). Once all client streams&sinks for a topic are dropped, the topic is being quit.

This builds on the client API added in #2258, but completely removes the dispatcher module, integrating its features directly into the gossip actor. See below for a short list of the API changes. The new API can be browsed here.

The refactor turned out bigger than initially intended, sorry for that, but I did not see a good way to reduce the scope.

What's still missing (can also be follow-ups)?:

Review the new public API
Align the client API to the iroh_gossip API. The GossipTopic can be made to work on both the client and the native API, as it only deals with streams and sinks.

Breaking Changes

iroh_gossip::dispatcher is removed with everything that was in it. use the new API from iroh_gossip::net::Gossip instead (see below).
iroh_gossip::net::Gossip methods changed:
- changed: join now returns a GossipTopic
- removed: broadcast, broadcast_neighbors, subscribe, subscribe_all, quit.
  - for subscribe use join instead, which returns a GossipTopic
  - for broadcast and broadcast_neighbors use the respective methods on GossipTopic .
  - quit is obsolete now, the topic will be quitted once all GossipTopic handles are dropped.
  - subscribe_all is no longer available
iroh_gossip::net::JoinTopicFut is removed (is now obsolete)

Notes & open questions

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-07-31T20:46:12Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2570/docs/iroh/

Last updated: 2024-08-05T18:44:20Z

matheus23 · 2024-08-01T06:16:20Z

Direct link to the gossip docs: https://n0-computer.github.io/iroh/pr/2570/docs/iroh/gossip/net/index.html :)

… read errors (#2572) ## Description The connection loop of iroh-gossip misused tokio-select by selecting over a future that is not cancellation safe. This means that if the timings are bad, the message reading future would be aborted midway in a message, and then restart by reading a length, which would then yield some random number because it would be reading some random bytes in the middle of a message. This means it would lead to random connection drops. ## Breaking Changes Backport from #2570  ## Notes & open questions  ## Change checklist - [x] Self-review. - [ ] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [ ] ~~Tests if relevant.~~ - [x] All breaking changes documented.

Frando · 2024-08-05T12:21:47Z

Tentatively marking ready for review. Still has TODOs (see description) but that could also be a followup.

dignifiedquire · 2024-08-05T15:18:04Z

n0-computer/iroh-ffi#173 is now green, so this definitely helps

dignifiedquire · 2024-08-05T15:20:12Z

iroh-docs/src/engine/live.rs


-use super::gossip::{GossipActor, ToGossipActor};
+// use super::gossip::{GossipActor, ToGossipActor};


dignifiedquire · 2024-08-05T15:21:10Z

iroh-docs/src/engine/live.rs

-        self.gossip_actor_tx
-            .send(ToGossipActor::Shutdown)
-            .await
-            .ok();


can't we wait for the gossip actor exit anymore?

there's no gossip actor anymore in the live engine! the GossipState is not a loop of its own, it just contains state and a joinset with the topic loop tasks. i will check again if that is terminated properly on shutdown.

the main Gossip is passed into the live engine, so should be shutdown outside it as well.

Pushed a commit that properly joins the topic task loops in the sync engine to catch panics. Also added a TODO to catch panics when aborthing other joinsets in the sync engine too.

dignifiedquire · 2024-08-05T15:22:46Z

iroh-gossip/src/net/handles.rs

+    /// Waits until we are connected to at least one node.
+    pub async fn joined(&mut self) -> Result<()> {
+        while self.neighbors.is_empty() {
+            let _ = self.try_next().await?;


shouldn't this be explicit about the type of event?

the Joined event is handled in try_next. But we could be more explict and return an error if any other event than Joined is returned, yes.

…opic (#2570) ## Description This PR changes the main public `iroh_gossip` to keep track of client-side gossip subscriptions. The `net::Gossip` struct now keeps track of client-side subscribers per topic, which are made up of a pair of two streams/channels: from the client to the actor a stream of updates (outgoing messages) and from the actor to the client a stream of events (incoming messages). Once all client streams&sinks for a topic are dropped, the topic is being quit. This builds on the client API added in #2258, but completely removes the `dispatcher` module, integrating its features directly into the gossip actor. See below for a short list of the API changes. The new API can be browsed [here](https://n0-computer.github.io/iroh/pr/2570/docs/iroh/gossip/net/index.html). The refactor turned out bigger than initially intended, sorry for that, but I did not see a good way to reduce the scope. What's still missing (can also be follow-ups)?: - [ ] Review the new public API - [ ] Align the client API to the iroh_gossip API. The `GossipTopic` can be made to work on both the client and the native API, as it only deals with streams and sinks. ## Breaking Changes * `iroh_gossip::dispatcher` is removed with everything that was in it. use the new API from `iroh_gossip::net::Gossip` instead (see below). * `iroh_gossip::net::Gossip` methods changed: * changed: `join` now returns a `GossipTopic` * removed: `broadcast`, `broadcast_neighbors`, `subscribe`, `subscribe_all`, `quit`. * for `subscribe` use `join` instead, which returns a `GossipTopic` * for `broadcast` and `broadcast_neighbors` use the respective methods on `GossipTopic` . * `quit` is obsolete now, the topic will be quitted once all `GossipTopic` handles are dropped. * `subscribe_all` is no longer available * `iroh_gossip::net::JoinTopicFut` is removed (is now obsolete) ## Notes & open questions  ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.

Frando added 8 commits July 31, 2024 21:40

wip: refactor gossip

535efdd

fix: connection loop was not cancellation safe

76b9556

wip: make things work again

bb7068e

fixup

3c7cc66

fixup

ab05a38

cleanup

8be9d71

fix: typos

967b8a3

fix: unrelated metrics feature flag hiccup

bac96ee

Frando changed the title ~~[wip] Refactor gossip API to expose subscriptions streams and sinks per topic~~ [wip] refactor(iroh-gossip) change API to expose subscriptions streams and sinks per topic Jul 31, 2024

fix: doclinks

4ac7fae

Frando changed the title ~~[wip] refactor(iroh-gossip) change API to expose subscriptions streams and sinks per topic~~ [wip] refactor(iroh-gossip) dispatch gossip events and updates by topic Jul 31, 2024

Frando changed the title ~~[wip] refactor(iroh-gossip) dispatch gossip events and updates by topic~~ [wip] refactor(iroh-gossip): dispatch gossip events and updates by topic Jul 31, 2024

Frando changed the title ~~[wip] refactor(iroh-gossip): dispatch gossip events and updates by topic~~ [wip] refactor(iroh-gossip)!: dispatch gossip events and updates by topic Jul 31, 2024

Frando added 2 commits August 1, 2024 00:30

tests: improve gossip client tests

19034dc

fix: keep two connections between peers

30b579e

Frando force-pushed the gossip-refactor branch from 8ba735b to 30b579e Compare July 31, 2024 22:49

Frando mentioned this pull request Aug 1, 2024

fix(iroh-gossip): connection loop misuses tokio::select! leading to read errors #2572

Merged

4 tasks

Frando marked this pull request as ready for review August 5, 2024 12:21

refactor: simplify public API

a8e7a59

dignifiedquire mentioned this pull request Aug 5, 2024

feat: update to iroh@0.22. n0-computer/iroh-ffi#173

Merged

Merge remote-tracking branch 'origin/main' into gossip-refactor

ddf9740

dignifiedquire reviewed Aug 5, 2024

View reviewed changes

fix(iroh-docs): properly abort gossip topics on shutdown

eefef7b

dignifiedquire approved these changes Aug 5, 2024

View reviewed changes

dignifiedquire enabled auto-merge August 5, 2024 18:56

dignifiedquire added this pull request to the merge queue Aug 5, 2024

dignifiedquire changed the title ~~[wip] refactor(iroh-gossip)!: dispatch gossip events and updates by topic~~ feat(iroh-gossip)!: dispatch gossip events and updates by topic Aug 5, 2024

Merged via the queue into main with commit bdc1c45 Aug 5, 2024
28 of 29 checks passed

dignifiedquire deleted the gossip-refactor branch August 5, 2024 19:27

dignifiedquire mentioned this pull request Aug 5, 2024

Unify gossip dispatchers / flaky test: gossip_smoke #2460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-gossip)!: dispatch gossip events and updates by topic #2570

feat(iroh-gossip)!: dispatch gossip events and updates by topic #2570

Frando commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

matheus23 commented Aug 1, 2024

Frando commented Aug 5, 2024

dignifiedquire commented Aug 5, 2024

dignifiedquire Aug 5, 2024

dignifiedquire Aug 5, 2024

Frando Aug 5, 2024 •

edited

Loading

Frando Aug 5, 2024

dignifiedquire Aug 5, 2024

Frando Aug 5, 2024


		use super::gossip::{GossipActor, ToGossipActor};
		// use super::gossip::{GossipActor, ToGossipActor};

feat(iroh-gossip)!: dispatch gossip events and updates by topic #2570

feat(iroh-gossip)!: dispatch gossip events and updates by topic #2570

Conversation

Frando commented Jul 31, 2024 • edited Loading

Description

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Jul 31, 2024 • edited Loading

matheus23 commented Aug 1, 2024

Frando commented Aug 5, 2024

dignifiedquire commented Aug 5, 2024

dignifiedquire Aug 5, 2024

Choose a reason for hiding this comment

dignifiedquire Aug 5, 2024

Choose a reason for hiding this comment

Frando Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Frando Aug 5, 2024

Choose a reason for hiding this comment

dignifiedquire Aug 5, 2024

Choose a reason for hiding this comment

Frando Aug 5, 2024

Choose a reason for hiding this comment

Frando commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

Frando Aug 5, 2024 •

edited

Loading