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.

Sign up for GitHub

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

Jump to bottom

feat(identify): keep connection alive while we are using it #3876

Merged
merged 23 commits into from
May 8, 2023
Merged

feat(identify): keep connection alive while we are using it #3876

merged 23 commits into from
May 8, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented May 4, 2023 •
edited
Loading

Description

Currently, the connection_keep_alive function of identify does not compute anything but its return value is set through the handler state machine. This is hard to understand and causes surprising behaviour because at the moment, we set KeepAlive::No as soon as the remote has answered our identify request. Depending on what else is happening on the connection, this might close the connection before we have successfully answered the remote's identify request.

To fix this, we now compute connection_keep_alive based on whether we are still using the connection.

Related: #3844.

Notes & open questions

Draft PR because it is stacked on top of #3851.

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
Move tests to tests/ directory
2149b03
@thomaseizinger
Rewrite first identify tests using libp2p-swarm-test
20a111c
@thomaseizinger
Rewrite another test
dc8dfd8
@thomaseizinger
Rewrite final test
ff41147
@thomaseizinger
Only listen on local interface in libp2p-swarm-test
336fcf3
@thomaseizinger
Assert listen and observed address
562a41f
@thomaseizinger
Ensure listen addresses are unique
edea5af
@thomaseizinger
Remove correct_transfer test
dd3c732 
This is now included in the smoke test
@thomaseizinger
Remove unnecessary dependencies
18012d7
@thomaseizinger
Delete dead code
2c5c9d3 
These files are not included in the module tree.
@thomaseizinger
Make tests more resilient
0182be7
@thomaseizinger
Revert "Only listen on local interface in libp2p-swarm-test"
d5bcef6 
This reverts commit 336fcf3.
@thomaseizinger
Assert using contains to avoid needing to know other interfaces
f3f5b69
@thomaseizinger
Merge branch 'master' into refactor/better-identify-tests
2f1fcff
@thomaseizinger
Merge branch 'master' into refactor/better-identify-tests
f332d69
@thomaseizinger
Fix flaky test
ee68201
@thomaseizinger
Fix more flakiness
499667f
@thomaseizinger
Merge branch 'master' into refactor/better-identify-tests
4f29de5
@thomaseizinger
Fix more flakiness
f878e94
@thomaseizinger
Keep connection alive while we are working on it
3a36bd4
This was referenced May 5, 2023
Closed
Closed
mxinden
mxinden approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Also now we have a good opinionated sample protocol showcasing how to best use rust-libp2p's keep-alive mechanism.

Base automatically changed from refactor/better-identify-tests to master May 8, 2023 03:39
@thomaseizinger thomaseizinger marked this pull request as ready for review May 8, 2023 08:52
@mergify mergify bot merged commit 8224f1a into master May 8, 2023
@mergify mergify bot deleted the feat/identify-keep-connection-alive branch May 8, 2023 09:31
@tcoratger tcoratger mentioned this pull request May 17, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request Jun 4, 2023
@tcoratger
feat(dcutr): keep connection alive while we are using it
a4450d4 
Similar to #3876, we now compute `connection_keep_alive` based on whether we are still using the connection, applied to the `dcutr` protocol.

Related: #3844.

Pull-Request: #3960.
@thomaseizinger thomaseizinger added this to the Simplify idle connection management milestone Sep 17, 2023
@thomaseizinger thomaseizinger mentioned this pull request Sep 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomaseizinger @mxinden