feat(kad): remove deprecated `Config::set_connection_idle_timeout` by leonzchang · Pull Request #4659 · libp2p/rust-libp2p

leonzchang · 2023-10-16T06:50:02Z

Description

Related: #3844.
Related: #4656.

Notes & open questions

some tests need to be fixed, after KeepAlive::Until being removed.

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 · 2023-10-16T07:00:47Z

some tests need to be fixed, after KeepAlive::Until being removed.

To fix these tests, we need to change the following lines of code:

rust-libp2p/protocols/kad/src/behaviour.rs

Lines 1911 to 1929 in c91ecbc

    
           // Peer's first connection. 
        
           if other_established == 0 { 
        
               // Queue events for sending pending RPCs to the connected peer. 
        
               // There can be only one pending RPC for a particular peer and query per definition. 
        
               for (peer_id, event) in self.queries.iter_mut().filter_map(|q| { 
        
                   q.inner 
        
                       .pending_rpcs 
        
                       .iter() 
        
                       .position(|(p, _)| p == &peer_id) 
        
                       .map(|p| q.inner.pending_rpcs.remove(p)) 
        
               }) { 
        
                   self.queued_events.push_back(ToSwarm::NotifyHandler { 
        
                       peer_id, 
        
                       event, 
        
                       handler: NotifyHandler::Any, 
        
                   }); 
        
               } 
        
               self.connected_peers.insert(peer_id);

What happens here is that after the connection is established, we tell the NetworkBehaviour about it. Without KeepAlive::Until however, the connection shuts down immediately after being established so these events are "too late". To avoid this, we need to "preload" the connection handler in the handle_established_{in,out}bound_connection callbacks. I am doing the same thing for libp2p-request-response in #4029.

This refactoring is backwards compatible so we can do that together with the deprecation of PR for set_connection_idle_timeout.

Let me know if that makes sense or you need more info :)

leonzchang · 2023-10-16T09:24:46Z

some tests need to be fixed, after KeepAlive::Until being removed.

Let me know if that makes sense or you need more info :)

I try to add preload_new_handler for Behaviour, but tests seems not going well, could you give me some hints?

thomaseizinger

Found two things so far, see if those fix the tests :)

protocols/kad/src/behaviour.rs

leonzchang · 2023-10-16T14:43:04Z

Found two things so far, see if those fix the tests :)

Big Thanks, I found that we have to set idle_timeout when initializing swarm in tests and example.

mergify · 2023-10-16T16:34:18Z

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

examples/ipfs-kad/Cargo.toml

thomaseizinger

Great! I left some inline comments.

protocols/kad/src/behaviour.rs

protocols/kad/src/behaviour/test.rs

protocols/kad/src/handler.rs

examples/ipfs-kad/src/main.rs

protocols/kad/src/behaviour.rs

…ed_events in preload_new_handler

thomaseizinger

Looks good, thank you!

Related: #3844. Related: #4659. Pull-Request: #4675.

mergify · 2023-10-20T01:22:30Z

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

thomaseizinger

Nice work, thank you!

protocols/kad/CHANGELOG.md

mergify · 2023-10-20T06:53:59Z

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

thomaseizinger added this to the v0.53.0 milestone Oct 16, 2023

thomaseizinger reviewed Oct 16, 2023

View reviewed changes

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

protocols/kad/src/behaviour.rs Show resolved Hide resolved

thomaseizinger reviewed Oct 16, 2023

View reviewed changes

examples/ipfs-kad/Cargo.toml Show resolved Hide resolved

thomaseizinger reviewed Oct 16, 2023

View reviewed changes

leonzchang added 6 commits October 17, 2023 11:11

remove idle_timeout and KeepAlive::Until

bc61086

update changelog

f4ca8bc

add preload_new_handler for Behaviour

ea62dd1

handle address_failed on connection established & remove pushing queu…

26257f7

…ed_events in preload_new_handler

fix test initializtion & fn on_connection_established

dd730da

fix ipfs kad example & bump version

2d58cbf

leonzchang force-pushed the feat/kad-remove-keep-until branch from 864943a to 2d58cbf Compare October 17, 2023 03:11

apply partial suggested fixes

78ebe38

leonzchang mentioned this pull request Oct 17, 2023

feat(kad): deprecate Config::set_connection_idle_timeout #4675

Merged

4 tasks

Merge branch 'master' into feat/kad-remove-keep-until

e36b204

thomaseizinger approved these changes Oct 17, 2023

View reviewed changes

mergify bot pushed a commit that referenced this pull request Oct 17, 2023

feat(kad): deprecate Config::set_connection_idle_timeout

411824a

Related: #3844. Related: #4659. Pull-Request: #4675.

thomaseizinger mentioned this pull request Oct 17, 2023

swarm: deprecate KeepAlive::Until #3844

Closed

This comment was marked as resolved.

Sign in to view

Merge branch 'master' into feat/kad-remove-keep-until

37095da

thomaseizinger marked this pull request as ready for review October 20, 2023 01:14

thomaseizinger and others added 3 commits October 20, 2023 12:27

Merge branch 'master' into feat/kad-remove-keep-until

2bf3b38

Merge branch 'master' into feat/kad-remove-keep-until

dccd146

Fix compile error

d56d975

thomaseizinger marked this pull request as draft October 20, 2023 04:29

restore substreams check in poll

0e4036a

Merge branch 'master' into feat/kad-remove-keep-until

b762e3f

thomaseizinger marked this pull request as ready for review October 20, 2023 06:30

leonzchang added 2 commits October 20, 2023 14:36

move keep alive computation to fn connection_keep_alive

909adf0

Merge branch 'master' into feat/kad-remove-keep-until

1de82ca

thomaseizinger approved these changes Oct 20, 2023

View reviewed changes

thomaseizinger changed the title ~~feat(kad): remove KeepAlive::Until~~ feat(kad): remove deprecated Config::set_connection_idle_timeout Oct 20, 2023

thomaseizinger reviewed Oct 20, 2023

View reviewed changes

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

Update protocols/kad/CHANGELOG.md

864e509

Merge branch 'master' into feat/kad-remove-keep-until

232ce1a

thomaseizinger added the send-it label Oct 20, 2023

thomaseizinger mentioned this pull request Oct 20, 2023

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

Merged

4 tasks

Merge branch 'master' into feat/kad-remove-keep-until

2a97ae5

mergify bot merged commit 1e3ffeb into libp2p:master Oct 20, 2023

Conversation

leonzchang commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes & open questions

Change checklist

Uh oh!

thomaseizinger commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonzchang commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leonzchang commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Oct 16, 2023

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

mergify bot commented Oct 20, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leonzchang commented Oct 16, 2023 •

edited

Loading

thomaseizinger commented Oct 16, 2023 •

edited

Loading

leonzchang commented Oct 16, 2023 •

edited

Loading

leonzchang commented Oct 16, 2023 •

edited

Loading