fix: garbage collect `Stream` when dropped #167

aschran · 2023-07-06T20:37:11Z

This fixes an issue where a Stream would never be removed until its connection is closed.

Resolves #166.

This fixes issue libp2p#166 where a `Stream` would never be removed until its connection is closed.

mxinden

Very well done @aschran. Thank you for reporting, debugging and fixing. I appreciate your work here.

I missed this in my review in #156 (comment).

Overall this looks good to me. Missing steps:

I would like to give @thomaseizinger a chance to review as well.
@aschran would you mind adding a changelog entry to CHANGELOG.md?

Again, well done. Thank you.

yamux/src/connection.rs

mxinden · 2023-07-07T10:00:38Z

This likely warrants a patch release (i.e. v0.11.1). I can get that done once we merged here.

…chran/dropstreams

mxinden · 2023-07-07T10:59:54Z

@aschran @thomaseizinger I merged latest master and added a test that fails on master without this patch.

Co-authored-by: Max Inden <[email protected]>

aschran · 2023-07-07T15:04:20Z

Committed your suggestions. Thanks for the review and confirmation + test! Will add the CHANGELOG entry shortly when I can get back to my computer.

thomaseizinger

Thank you!

Ironically, the fact that all of Stream was shared bugged me already in the past because it is conceptually hard to understand. The fact that it now actually caused a bug annoys me that I didn't follow my gut feeling there :)

I made one suggestion on how I think we could make this a bit cleaner. Let me know if you are up for it, otherwise happy to merge this as is once CI passes!

CHANGELOG.md

yamux/src/connection.rs

thomaseizinger · 2023-07-09T09:19:54Z

yamux/src/connection.rs

@@ -350,7 +353,7 @@ struct Active<T> {
    socket: Fuse<frame::Io<T>>,
    next_id: u32,

-    streams: IntMap<StreamId, Stream>,
+    streams: IntMap<StreamId, Arc<Mutex<stream::Shared>>>,


I wonder if it would turn out cleaner if Shared would hold Arc<Mutex> internally? That would remove the boilerplate around locking and some noise from the type signatures.

Would prefer to merge the fix and leave this for future work if that's ok with you, since it looks like a bit more involved of a change

Co-authored-by: Thomas Eizinger <[email protected]>

thomaseizinger · 2023-07-09T17:07:46Z

Can you look at the CI failures? Otherwise LGTM!

aschran · 2023-07-10T03:31:12Z

Can you look at the CI failures? Otherwise LGTM!

I'll admit I'm not sure what's up with those. If I run cargo build on this branch locally it works fine for me. The errors in CI suggest it's not running on the same code I'm looking at? yamux/src/connection.rs is only 927 lines long and yet errors are showing lines 942-943? It doesn't seem I have access to trigger a re-run of CI myself.

…chran/dropstreams

mxinden · 2023-07-10T04:52:02Z

Our CI first merges the target branch, here master, and then executes the tests.

We recently merged #153. While git does not identify it as a merge conflict, it does conflict with this pull request.

I pushed a merge resolving the non-git-merge-conflict conflicts. @thomaseizinger this moves the is_outbound method inline and duplicates the is_pending_ack on Shared.

aschran · 2023-07-10T13:10:54Z

Thanks for helping explain & fix Max :)

mxinden

Merging here.

@thomaseizinger let me know in case you would like to see any follow-ups given the latest changes.

@aschran once more, thank you for the work. Let me know in case you would like me to backport this change to v0.11.

aschran · 2023-07-11T03:28:59Z

@mxinden yes, if you have time to backport that would be great! Appreciate the diligent review from you both.

thomaseizinger · 2023-07-11T15:40:55Z

@thomaseizinger let me know in case you would like to see any follow-ups given the latest changes.

None that are critical for a release.

mxinden · 2023-07-20T14:01:02Z

@aschran released happened through #169. Let me know if that is good enough for you.

aschran · 2023-07-20T16:16:01Z

That works, thanks for letting me know!

… API (#320) This PR relies on the libp2p-yamux crate for the core functionality of our multiplexer. The main goal is to bring complete compatibility between libp2p and litep2p on the yamux layer, and remove 90% of the yamux code in favor of the upstream implementation while keeping the controller API in place. The upstream crate brings in multiple fixes for substreams while minimally impacting our dependency tree. The downside of the upstream implementation is that the controller API has been removed. Adjusting to the new API would be a massive breaking change for all transport layers. Therefore, we keep the controller API which integrates seamlessly with the upstream yamux. No other changes were present to the controller API in the upstream implementation. ### Yamux changelog The changelog includes the fixes from the upstream since the moment we have inlined the crate in litep2p: ``` # 0.13.4 - Fix sending pending frames after closing. See [PR 194](libp2p/rust-yamux#194). # 0.13.3 - Wake up readers after setting the state to RecvClosed to not miss EOF. See [PR 190](libp2p/rust-yamux#190). - Use `web-time` instead of `instant`. See [PR 191](libp2p/rust-yamux#191). # 0.13.2 - Bound `Active`'s `pending_frames` to enforce backpressure. See [460baf2](libp2p/rust-yamux@460baf2) # 0.13.1 - Fix WASM support using `instant::{Duration, Instant}` instead of `std::time::{Duration, Instant}`. See [PR 179](libp2p/rust-yamux#179). # 0.13.0 - Introduce dynamic stream receive window auto-tuning. While low-resourced deployments maintain the benefit of small buffers, high resource deployments eventually end-up with a window of roughly the bandwidth-delay-product (ideal) and are thus able to use the entire available bandwidth. See [PR 176](libp2p/rust-yamux#176) for performance results and details on the implementation. - Remove `WindowUpdateMode`. Behavior will always be `WindowUpdateMode::OnRead`, thus enabling flow-control and enforcing backpressure. See [PR 178](libp2p/rust-yamux#178). # 0.12.1 - Deprecate `WindowUpdateMode::OnReceive`. It does not enforce flow-control, i.e. breaks backpressure. Use `WindowUpdateMode::OnRead` instead. See [PR #177](libp2p/rust-yamux#177). # 0.12.0 - Remove `Control` and `ControlledConnection`. Users have to move to the `poll_` functions of `Connection`. See [PR #164](libp2p/rust-yamux#164). - Fix a bug where `Stream`s would not be dropped until their corresponding `Connection` was dropped. See [PR #167](libp2p/rust-yamux#167). ``` ### Next Steps - [x] deployment in versi-net and monitor metrics / CPU impact (extensively test this) cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]>

Only save stream Shared state in Active<T>

ed1bd31

This fixes issue libp2p#166 where a `Stream` would never be removed until its connection is closed.

mxinden reviewed Jul 7, 2023

View reviewed changes

yamux/src/connection.rs Outdated Show resolved Hide resolved

yamux/src/connection.rs Outdated Show resolved Hide resolved

yamux/src/connection.rs Outdated Show resolved Hide resolved

mxinden added 2 commits July 7, 2023 19:26

Merge branch 'master' of https://github.com/libp2p/rust-yamux into as…

82554e0

…chran/dropstreams

test: add close_through_drop_of_stream_propagates_to_remote

7959b5d

aschran and others added 3 commits July 7, 2023 11:03

Update yamux/src/connection.rs

b33c80b

Co-authored-by: Max Inden <[email protected]>

Update yamux/src/connection.rs

801a874

Co-authored-by: Max Inden <[email protected]>

Update yamux/src/connection.rs

69b88e0

Co-authored-by: Max Inden <[email protected]>

Add CHANGELOG.md message

d5400c5

thomaseizinger approved these changes Jul 9, 2023

View reviewed changes

aschran and others added 2 commits July 9, 2023 10:04

Fix build

60ab872

Update CHANGELOG.md

4931e46

Co-authored-by: Thomas Eizinger <[email protected]>

thomaseizinger approved these changes Jul 9, 2023

View reviewed changes

Merge branch 'master' of https://github.com/libp2p/rust-yamux into as…

df03052

…chran/dropstreams

mxinden approved these changes Jul 11, 2023

View reviewed changes

mxinden changed the title ~~Only save stream Shared state in Active<T>~~ fix: garbage collect Stream when dropped Jul 11, 2023

mxinden merged commit dcff3d5 into libp2p:master Jul 11, 2023

mxinden mentioned this pull request Jul 20, 2023

chore: prepare v0.12.0 #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: garbage collect `Stream` when dropped #167

fix: garbage collect `Stream` when dropped #167

aschran commented Jul 6, 2023 •

edited by thomaseizinger

Loading

mxinden left a comment

mxinden commented Jul 7, 2023

mxinden commented Jul 7, 2023

aschran commented Jul 7, 2023 •

edited

Loading

thomaseizinger left a comment

thomaseizinger Jul 9, 2023

aschran Jul 9, 2023

thomaseizinger commented Jul 9, 2023

aschran commented Jul 10, 2023

mxinden commented Jul 10, 2023

aschran commented Jul 10, 2023

mxinden left a comment

aschran commented Jul 11, 2023

thomaseizinger commented Jul 11, 2023

mxinden commented Jul 20, 2023

aschran commented Jul 20, 2023

fix: garbage collect Stream when dropped #167

fix: garbage collect Stream when dropped #167

Conversation

aschran commented Jul 6, 2023 • edited by thomaseizinger Loading

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Jul 7, 2023

mxinden commented Jul 7, 2023

aschran commented Jul 7, 2023 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Jul 9, 2023

Choose a reason for hiding this comment

aschran Jul 9, 2023

Choose a reason for hiding this comment

thomaseizinger commented Jul 9, 2023

aschran commented Jul 10, 2023

mxinden commented Jul 10, 2023

aschran commented Jul 10, 2023

mxinden left a comment

Choose a reason for hiding this comment

aschran commented Jul 11, 2023

thomaseizinger commented Jul 11, 2023

mxinden commented Jul 20, 2023

aschran commented Jul 20, 2023

fix: garbage collect `Stream` when dropped #167

fix: garbage collect `Stream` when dropped #167

aschran commented Jul 6, 2023 •

edited by thomaseizinger

Loading

aschran commented Jul 7, 2023 •

edited

Loading