fix(swarm): keep connections alive while active streams exist #4595

leonzchang · 2023-10-05T10:22:02Z

Description

Resolves: #4520.
Related: #4306.

Notes & open questions

Much thanks to @thomaseizinger for the advices!

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Some initial thoughts, thank you for getting this started!

Before we keep on going with the implementation for too long, I'd like us to update the docs on ConnectionHandler::connection_keep_alive to reflect what we are changing here. If you give that a start, it will show up in the diff and I can help out with some comments :)

swarm/src/lib.rs

swarm/src/connection.rs

swarm/src/stream.rs

swarm/src/connection.rs

swarm/src/stream.rs

thomaseizinger

Great progress!

Now, to check that this actually works, we need to go through all implementations of ConnectionHandler and remove any code that currently duplicates this kind of stream tracking.

For example here:

rust-libp2p/protocols/identify/src/handler.rs

Lines 317 to 323 in 7d1d67c

    
           fn connection_keep_alive(&self) -> KeepAlive { 
        
               if !self.active_streams.is_empty() { 
        
                   return KeepAlive::Yes; 
        
               } 
        
               KeepAlive::No 
        
           }

This is now unnecessary and can just return KeepAlive::No.

swarm/Cargo.toml

protocols/ping/CHANGELOG.md

swarm/CHANGELOG.md

swarm/src/connection.rs

swarm/src/handler.rs

swarm/src/stream.rs

mergify · 2023-10-10T06:58:11Z

This pull request has merge conflicts. Could you please resolve them @leonzchang? 🙏

thomaseizinger

Great progress!

protocols/identify/src/handler.rs

mergify · 2023-10-13T04:34:33Z

This pull request has merge conflicts. Could you please resolve them @leonzchang? 🙏

thomaseizinger

Thanks for making the follow-up changes!

Whilst I think we can overall merge these changes (modulo new comments), it would be much cleaner if we'd get rid of KeepAlive::Until first. See #3844. Are you interested in helping out on that front too? :)

protocols/kad/src/handler.rs

protocols/perf/src/client/handler.rs

protocols/relay/src/priv_client/handler.rs

protocols/relay/src/behaviour/handler.rs

protocols/identify/CHANGELOG.md

protocols/kad/CHANGELOG.md

leonzchang · 2023-10-13T06:58:36Z

Thanks for making the follow-up changes!

Whilst I think we can overall merge these changes (modulo new comments), it would be much cleaner if we'd get rid of KeepAlive::Until first. See #3844. Are you interested in helping out on that front too? :)

Do you mean remove KeepAlive::Until in /protocols before this branch merge?
Aye, I can try it out.

mergify · 2023-10-15T08:41:41Z

This pull request has merge conflicts. Could you please resolve them @leonzchang? 🙏

protocols/kad/src/handler.rs

thomaseizinger · 2023-10-15T08:55:31Z

@mxinden Would be great to get your opinion here. We can merge this early by only removing the conditions for stream checks in the protocols where we no longer use KeepAlive::Until. Then this could essentially be released as a patch-release.

I am tempted to do that because it just means that we are doing the bookkeeping twice but it doesn't hurt. We can always remove it at a later stage.

mergify · 2023-10-16T16:35:42Z

This pull request has merge conflicts. Could you please resolve them @leonzchang? 🙏

leonzchang · 2023-10-18T09:00:42Z

rebase master

thomaseizinger

Great progress!

I've left a few more comments. Hope you don't mind :)
Also, if you are interested, we maintainers will meet later today: #4713. If you want, you can present your work here or also just listen in :)

swarm/src/stream.rs

protocols/relay/src/behaviour/handler.rs

protocols/relay/src/priv_client/handler.rs

swarm/CHANGELOG.md

swarm/src/handler.rs

swarm/src/handler/multi.rs

swarm/src/handler/pending.rs

swarm/src/handler/select.rs

swarm/src/keep_alive.rs

thomaseizinger

I updated the docs a bit. This is ready from my end, thank you so much for helping out with this! I am super stoked to land this 🎉

@mxinden Please have a look!

mxinden

Lots of simplifications! Well done. Thanks. Also great work on the documentation.

I have a couple of comments. I am sorry in case these have been discussed before.

protocols/gossipsub/src/behaviour.rs

protocols/gossipsub/src/handler.rs

protocols/ping/src/handler.rs

protocols/relay/src/behaviour/handler.rs

swarm/CHANGELOG.md

swarm/src/connection.rs

swarm/src/handler.rs

mergify · 2023-10-24T17:31:43Z

This pull request has merge conflicts. Could you please resolve them @leonzchang? 🙏

thomaseizinger

Great work!

mxinden

Well done!

mxinden · 2023-10-25T08:26:55Z

@romanb you might enjoy seeing your proposal from a couple of years ago finally implemented.

thomaseizinger reviewed Oct 6, 2023

View reviewed changes

swarm/src/lib.rs Outdated Show resolved Hide resolved

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

swarm/src/stream.rs Outdated Show resolved Hide resolved

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

swarm/src/stream.rs Outdated Show resolved Hide resolved

leonzchang marked this pull request as ready for review October 6, 2023 07:31

leonzchang requested a review from thomaseizinger October 6, 2023 08:10

thomaseizinger reviewed Oct 9, 2023

View reviewed changes

leonzchang force-pushed the feat/stream-counter branch from 8f26fed to b98be2b Compare October 11, 2023 06:39

leonzchang requested a review from thomaseizinger October 11, 2023 07:12

thomaseizinger reviewed Oct 11, 2023

View reviewed changes

protocols/identify/src/handler.rs Show resolved Hide resolved

leonzchang requested a review from thomaseizinger October 12, 2023 05:41

leonzchang force-pushed the feat/stream-counter branch from c931771 to 67750db Compare October 13, 2023 04:56

thomaseizinger reviewed Oct 13, 2023

View reviewed changes

protocols/kad/src/handler.rs Outdated Show resolved Hide resolved

protocols/perf/src/client/handler.rs Outdated Show resolved Hide resolved

protocols/relay/src/priv_client/handler.rs Outdated Show resolved Hide resolved

protocols/relay/src/behaviour/handler.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Oct 13, 2023

View reviewed changes

protocols/identify/CHANGELOG.md Outdated Show resolved Hide resolved

thomaseizinger reviewed Oct 13, 2023

View reviewed changes

protocols/kad/CHANGELOG.md Outdated Show resolved Hide resolved

leonzchang requested a review from thomaseizinger October 13, 2023 06:58

thomaseizinger mentioned this pull request Oct 13, 2023

fix(swarm): prevent overflow in keep-alive computation #4644

Merged

4 tasks

thomaseizinger reviewed Oct 15, 2023

View reviewed changes

protocols/kad/src/handler.rs Outdated Show resolved Hide resolved

leonzchang force-pushed the feat/stream-counter branch from 78394dc to 71f3bc1 Compare October 16, 2023 05:27

thomaseizinger mentioned this pull request Oct 16, 2023

feat(kad): remove deprecated Config::set_connection_idle_timeout #4659

Merged

4 tasks

leonzchang force-pushed the feat/stream-counter branch from 5c272d8 to 5bf6f0d Compare October 18, 2023 09:00

leonzchang added 5 commits October 18, 2023 17:04

add no_keep_alive fn to Stream and counter to Connection

e5aaa04

call fn no_keep_alive when ping protocol stream received

55525cc

apply suggested fixes

340ecc9

apply suggested fixes

55b2c6d

update root Cargo.toml

e07e274

leonzchang added 4 commits October 24, 2023 12:14

fix clippy lint

991ef47

fix clippy lint

18b7314

Merge branch 'master' into feat/stream-counter

6fc1b0b

revert 3e75728 wrong docs

69a4a8b

thomaseizinger reviewed Oct 24, 2023

View reviewed changes

leonzchang added 2 commits October 24, 2023 14:27

apply suggested fixes

3d10eb4

Merge branch 'master' into feat/stream-counter

c79537b

leonzchang requested a review from thomaseizinger October 24, 2023 06:31

leonzchang and others added 4 commits October 24, 2023 14:39

fixed test

47a2b4e

Update docs

4ad1117

Remove obsolete docs

c963452

Update changelog

a58584d

thomaseizinger approved these changes Oct 24, 2023

View reviewed changes

thomaseizinger and others added 2 commits October 24, 2023 18:36

Fix doc link

0d02ccc

replace Arc<()> to ActiveStreamCounter in swarm

0c9e3ee

mxinden reviewed Oct 24, 2023

View reviewed changes

leonzchang added 4 commits October 25, 2023 11:58

apply suggested fixes

92823e1

Merge branch 'master' into feat/stream-counter

16548b1

merge completed

1aaa6a0

Merge branch 'master' into feat/stream-counter

17d4513

leonzchang requested a review from mxinden October 25, 2023 05:09

leonzchang added 3 commits October 25, 2023 15:23

Merge branch 'master' into feat/stream-counter

51c2df6

wrap Arc to a new type & refactor stream ignore_for_keep_alive

41662a4

fix clippy error

46d8216

thomaseizinger approved these changes Oct 25, 2023

View reviewed changes

mxinden approved these changes Oct 25, 2023

View reviewed changes

mxinden added the send-it label Oct 25, 2023

mergify bot merged commit fcd410a into libp2p:master Oct 25, 2023
71 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(swarm): keep connections alive while active streams exist #4595

fix(swarm): keep connections alive while active streams exist #4595

leonzchang commented Oct 5, 2023 •

edited

Loading

thomaseizinger left a comment

thomaseizinger left a comment

mergify bot commented Oct 10, 2023

thomaseizinger left a comment

mergify bot commented Oct 13, 2023

thomaseizinger left a comment

leonzchang commented Oct 13, 2023 •

edited

Loading

mergify bot commented Oct 15, 2023

thomaseizinger commented Oct 15, 2023

mergify bot commented Oct 16, 2023

leonzchang commented Oct 18, 2023

thomaseizinger left a comment

thomaseizinger left a comment

mxinden left a comment

mergify bot commented Oct 24, 2023

thomaseizinger left a comment

mxinden left a comment

mxinden commented Oct 25, 2023

	fn connection_keep_alive(&self) -> KeepAlive {
	if !self.active_streams.is_empty() {
	return KeepAlive::Yes;
	}

	KeepAlive::No
	}

fix(swarm): keep connections alive while active streams exist #4595

fix(swarm): keep connections alive while active streams exist #4595

Conversation

leonzchang commented Oct 5, 2023 • edited Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 10, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 13, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

leonzchang commented Oct 13, 2023 • edited Loading

mergify bot commented Oct 15, 2023

thomaseizinger commented Oct 15, 2023

mergify bot commented Oct 16, 2023

leonzchang commented Oct 18, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 24, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Oct 25, 2023

leonzchang commented Oct 5, 2023 •

edited

Loading

leonzchang commented Oct 13, 2023 •

edited

Loading