Skip to content

Conversation

@pradovic
Copy link
Contributor

@pradovic pradovic commented Feb 27, 2025

Description

There is a special Idle connection timeout section in the ping tutorial which says that we should include connection timeout settings, but the settings are not included.

I have checked previous versions and they had it so it was either lost by mistake, or the section is not relevant anymore.
This is a very minor PR to update the docs.

Notes & open questions

N/A

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

@elenaf9
Copy link
Member

elenaf9 commented Feb 27, 2025

Good catch!
The line was removed in #4967 because the default connection timeout was changed from 0s (connection times out immediately) to 10s.

I think we should keep this section with the readded setting and just fix the following two sentences:

@elenaf9 elenaf9 changed the title chore: fix libp2p ping tutorial connection timeout docs(tutorial): fix libp2p ping tutorial connection timeout Feb 27, 2025
@pradovic
Copy link
Contributor Author

Good catch! The line was removed in #4967 because the default connection timeout was changed from 0s (connection times out immediately) to 10s.

I think we should keep this section with the readded setting and just fix the following two sentences:

* https://github.com/libp2p/rust-libp2p/blob/67425531aef5f885efc964b55075d96639d1da7c/libp2p/src/tutorials/ping.rs#L208
  
  -> `//! The default connection timeout is 10 seconds.`

* https://github.com/libp2p/rust-libp2p/blob/67425531aef5f885efc964b55075d96639d1da7c/libp2p/src/tutorials/ping.rs#L213
  
  -> `//! Thus, without any other behaviour in place, we would not be able to observe any pings after 10s.`

I see, it makes 100% sense now. Fixed in 8429e8c. Thank you!

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi, the rustfmt job is still failing

@pradovic
Copy link
Contributor Author

Hi, the rustfmt job is still failing

Oups, did not check with nightly 🙈 Fixed. Thanks!

Copy link
Member

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thanks @pradovic!

@mergify mergify bot merged commit 37aa175 into libp2p:master Feb 28, 2025
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants