feat(ping): making the ignoring of the first error configurable #5005
Conversation
stormshield-frb
force-pushed
the
feat/add-configurable-silent-ping-error
branch
from
c1e036c
to
092ec64
Compare
There was a problem hiding this comment.
I am overall okay with this but I think we should name this something like a "compatibility"-mode with implementations that use one stream per ping.
Maybe something like:
pub fn with_allow_new_stream_per_ping() { }In the rust docs, we can then explain that this is for compatibility with other implementations.
I don't actually know what the recommended way is. In general, we prefer short-lived streams. I think we could actually remove a lot of this code if we'd move away from a long-lived ping stream. Old rust-libp2p clients are already compatible with this.
@mxinden What do you think? Do you have any historical knowledge on this?
No historical knowledge beyond what is posted above. I am fine with either direction. Preference for no additional config flag if possible. |
Okay thank you! Re-reading the spec, I think it is safe to move away from a long-lived stream and instead only ever send a single ping and close the stream afterwards. @stormshield-frb Would you mind re-writing this PR to do that? I think that should also solve #5004. |
|
About moving away from long-lived stream, will it not be a costly to open and close streams with |
Yamux streams are similarly cheap to QUIC streams! :) |
|
This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏 |
Description
Fix #5004
Notes & open questions
Instead of using a
silent_first_error: bool, we could also use aignored_errors_nb: usize. However, since there was previously aself.config.max_failuresand it was removed, I did not re-do it. Of course, if it is preferred I can do it.Change checklist