-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add backpressure to Channel receivers #412
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EpicEric
force-pushed
the
backpressure-fix
branch
from
December 8, 2024 01:59
5df22e9
to
2b3656f
Compare
Fixes Eugeny#392. Simply put, this changes its `UnboundedReceiver` from `unbounded_channel` with a `Receiver` from `channel(...)`, with a configurable value in `channel_buffer_size` of 100. This means that, for each channel that's created, it can hold up to 100 `Msg` objects, which are at most as big as the window size plus metadata/indirection pointers. This is enough to force messages to be read from the underlying `TcpStream` only as quickly as the server/client is able to handle them. This has been tested with the example given in Eugeny#392, including a modification to slowly read into a non-expanding buffer, and appears to fix the issue that's been pointed out. Some other considerations: - It is still possible to cause memory to explode by opening multiple channels, although that might be intentional on the user's part. In this case, it's up to the user to handle when channels get created or not. - The limited buffering also means that control `Msg`s (eg. `Eof`) will also be delayed by the transport layer backpressure, although this might be expected by the SSH standard. Reading through RFC 4254, I couldn't find any mention of non-`Data`/`ExtendedData` messages having to be dealt differently, but I don't think it's even possible to do so over TCP. If this assumption is wrong, this might require a separate `mpsc::unbounded_channel` just for control messages. **BREAKING CHANGES**: - This removes `fn send_channel_msg`, which doesn't seem to be used anywhere in this library, but is part of the publicly exposed API nonetheless. - This adds the configuration value `channel_buffer_size` to both servers and clients that allows setting how big the buffer size should be.
EpicEric
force-pushed
the
backpressure-fix
branch
from
December 9, 2024 19:06
2b3656f
to
0512b24
Compare
Eugeny
reviewed
Dec 12, 2024
Eugeny
reviewed
Dec 12, 2024
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #392.
Simply put, this changes its
UnboundedReceiver
fromunbounded_channel
with aReceiver
fromchannel(...)
, with aconfigurable value in
channel_buffer_size
of 100. This means that, foreach channel that's created, it can hold up to 100
Msg
objects, whichare at most as big as the window size plus metadata/indirection
pointers.
This is enough to force messages to be read from the underlying
TcpStream
only as quickly as the server/client is able to handle them.This has been tested with the example given in #392, including a
modification to slowly read into a non-expanding buffer, and appears to
fix the issue that's been pointed out.
Some other considerations:
channels, although that might be intentional on the user's part. In
this case, it's up to the user to handle when channels get created or
not.
Msg
s (eg.Eof
) willalso be delayed by the transport layer backpressure, although this
might be expected by the SSH standard. Reading through RFC 4254, I
couldn't find any mention of non-
Data
/ExtendedData
messages havingto be dealt differently, but I don't think it's even possible to do so
over TCP. If this assumption is wrong, this might require a separate
mpsc::unbounded_channel
just for control messages.BREAKING CHANGES:
fn send_channel_msg
, which doesn't seemto be used anywhere in this library, but is part of the publicly
exposed API nonetheless.
channel_buffer_size
to bothservers and clients that allows setting how big the buffer size should
be.