feat: don't allow to open more than 256 unacknowledged streams #153
Conversation
yamux/src/connection/stream.rs
Outdated
|
|
||
| pub fn is_acknowledged(&self) -> bool { | ||
| self.shared().acknowledged | ||
| } |
There was a problem hiding this comment.
Why expose this to the user? For the test above only? If so, can we move the test instead?
| pub fn is_acknowledged(&self) -> bool { | |
| self.shared().acknowledged | |
| } | |
| pub(crate) fn is_acknowledged(&self) -> bool { | |
| self.shared().acknowledged | |
| } |
There was a problem hiding this comment.
I think in the context of rust-yamux, this is useful information. Yes, it also allows us to keep the test next to all the others.
I am not too worried about this being in the public API because it is encapsulated within libp2p-yamux anyway. A nice benefit is that our tests are written against a more stable interface instead of reaching into internal data structures.
There was a problem hiding this comment.
I think in the context of
rust-yamux, this is useful information
In what scenario would a user use it?
There was a problem hiding this comment.
Until the point where the stream is not acknowledged, this is essentially equivalent to asking a QUIC connection whether it is 0RTT. Anything you write to a stream before it is acknowledged might be discarded by the remote.
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Max Inden <[email protected]>
thomaseizinger
changed the title
yamux/src/connection/stream.rs
Outdated
| // technically, the frame hasn't been sent yet on the wire but from the perspective of this data structure, we've queued the frame for sending | ||
| if frame.header().flags().contains(ACK) { | ||
| self.set_acknowledged(); | ||
| } |
There was a problem hiding this comment.
This is confusing to me. Do I understand correctly that this sets an inbound stream to be acknowledged? Why would the local state machine care whether an inbound stream is acknowledged or not?
There was a problem hiding this comment.
We can omit this but it means that the flag is only used by outbound streams which IMO is even more confusing.
I guess I could rename it?
There was a problem hiding this comment.
Ah, I remember why I am doing it for both. I needed that for the test around timing, i.e. when a stream gets acknowledged.
There was a problem hiding this comment.
That is fine by me. Though could you please document it for future us?
This doesn't actually save any LoC.
|
This is still buggy for some reason. I am not sure if it has to do with the recently merged changes and the GC fix. Will put on hold for now. |
This test is covered by `prop_send_recv_multi`.
|
I've merged #164 into here and now this PR passes all tests 🥳 |
yamux/src/connection/stream.rs
Outdated
|
|
||
| pub fn is_acknowledged(&self) -> bool { | ||
| self.shared().acknowledged | ||
| } |
There was a problem hiding this comment.
I think in the context of
rust-yamux, this is useful information
In what scenario would a user use it?
yamux/src/connection/stream.rs
Outdated
| // technically, the frame hasn't been sent yet on the wire but from the perspective of this data structure, we've queued the frame for sending | ||
| if frame.header().flags().contains(ACK) { | ||
| self.set_acknowledged(); | ||
| } |
There was a problem hiding this comment.
That is fine by me. Though could you please document it for future us?
Resolves #150.