-
Notifications
You must be signed in to change notification settings - Fork 43
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.
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
src/lib: Default to WindowUpdateMode::OnRead #120
Conversation
Default to `WindowUpdateMode::OnRead`, thus enabling full Yamux flow-control, exercising back pressure on senders, preventing stream resets due to reaching the buffer limit. See the [`WindowUpdateMode` documentation] for details, especially the section on deadlocking when sending data larger than the receivers window. [`WindowUpdateMode` documentation]: https://docs.rs/yamux/0.9.0/yamux/enum.WindowUpdateMode.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the intricacies here.
It does seem like in most cases the deadlock risk is unlikely compared the benefits in setting this to default, so makes sense to me.
//CC @ec2 and @olibearo as you are currently using https://github.com/ChainSafe/forest/blob/main/node/forest_libp2p/src/service.rs#L381 //CC @koivunej for rust-ipfs: https://github.com/rs-ipfs/rust-ipfs/blob/master/src/p2p/transport.rs#L31 //CC @dvc94ch for ipfs-embed: https://github.com/ipfs-rust/ipfs-embed/blob/master/src/net/mod.rs#L93 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @mxinden! Per your explanation this sounds very much a reasonable default but I don't have knowledge on yamux internals except for what I've learned by reading random issues.
Seems ok. It's not a breaking change as it only affects the receiver behavior if I understand correctly. Thanks! |
Continuation of libp2p#120. Preparation for libp2p#175. Also removes dead-lock warning for `WindowUpdateMode::OnRead`. With the restructuring of `Connection::poll`, one reads from the socket when writing is blocked. Thus the deadlock can not occur.
Default to
WindowUpdateMode::OnRead
, thus enabling full Yamuxflow-control, exercising back pressure on senders, preventing stream
resets due to reaching the buffer limit.
See the
WindowUpdateMode
documentation for details, especially thesection on deadlocking when sending data larger than the receivers
window.
This would prevent surprises like libp2p/rust-libp2p#2089.