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

Add backpressure to Channel receivers #412

Merged
merged 7 commits into from
Dec 12, 2024
Merged

Conversation

EpicEric
Copy link
Contributor

@EpicEric EpicEric commented Dec 7, 2024

Fixes #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 #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 Msgs (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 EpicEric marked this pull request as ready for review December 8, 2024 01:53
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.
russh/src/server/mod.rs Outdated Show resolved Hide resolved
russh/src/client/mod.rs Outdated Show resolved Hide resolved
@Eugeny Eugeny merged commit f89c19c into Eugeny:main Dec 12, 2024
6 checks passed
@Eugeny
Copy link
Owner

Eugeny commented Dec 12, 2024

Thank you!

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 this pull request may close these issues.

Channel lacks high watermark flow control
2 participants