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.

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

Track the accept backlog and add an upper limit #150

Closed
thomaseizinger opened this issue Dec 16, 2022 · 3 comments · Fixed by #153
Closed

Track the accept backlog and add an upper limit #150

thomaseizinger opened this issue Dec 16, 2022 · 3 comments · Fixed by #153

Comments

@thomaseizinger
Copy link
Contributor

See libp2p/specs#471 (comment).

@thomaseizinger
Copy link
Contributor Author

Now that we have landed the refactoring, this is relatively easy to implement. We just need to track the ACK backlog internally (i.e. the number of streams we have opened but the remote has not yet acknowledged). Once that number reaches 256, we should return Poll::Pending from poll_new_outbound: https://github.com/libp2p/rust-yamux/blob/master/yamux/src/connection.rs#L160

@mxinden
Copy link
Member

mxinden commented May 8, 2023

We just need to track the ACK backlog internally (i.e. the number of streams we have opened but the remote has not yet acknowledged). Once that number reaches 256, we should return Poll::Pending from poll_new_outbound: https://github.com/libp2p/rust-yamux/blob/master/yamux/src/connection.rs#L160

This would be step (1), i.e. the sender side.

Step (2) would be to implement the listener side. If I am not mistaken we acknowledge new incoming streams right away (please double check whether this is correct). Instead we should only acknowledge a new incoming stream once it has been passed to the user via Connection::poll_next_inbound.

@thomaseizinger
Copy link
Contributor Author

We just need to track the ACK backlog internally (i.e. the number of streams we have opened but the remote has not yet acknowledged). Once that number reaches 256, we should return Poll::Pending from poll_new_outbound: https://github.com/libp2p/rust-yamux/blob/master/yamux/src/connection.rs#L160

This would be step (1), i.e. the sender side.

Step (2) would be to implement the listener side. If I am not mistaken we acknowledge new incoming streams right away (please double check whether this is correct). Instead we should only acknowledge a new incoming stream once it has been passed to the user via Connection::poll_next_inbound.

I think we implicitly do that already by acknowledging it with the first Data frame that is sent on the stream, i.e. not until the user actually uses the stream.

We should confirm this with a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants