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 send buffer #97

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

cbrewster
Copy link
Member

Why

Currently the send buffer has no backpressure, so if a user of the river client sends too many messages too quickly, the send buffer will overflow and the client will error out.

Rather than relying on users to rate limit themselves, we introduce backpressure so client naturally will only send messages as fast as the river client/server is able to consume them.

What changed

  • Add a condvar to the message buffer to allow waiting for buffer space to come available when the buffer is full
  • Add a closed bit to the message buffer so when a session is shut down, we have a way of unblocking all the futures that are stuck waiting for space in the buffer
  • Close the message buffer when the session is closed

Test plan

  • Added a test which sends 2 * MAX_MESSAGE_BUFFER_SIZE messages to a river server. Previously this resulted in an error.
  • Added some unit tests for the MessageBuffer to ensure backpressure works and closing works

@cbrewster cbrewster requested a review from a team as a code owner October 8, 2024 18:52
@cbrewster cbrewster requested review from blast-hardcheese and removed request for a team October 8, 2024 18:52
@cbrewster
Copy link
Member Author

cbrewster commented Oct 8, 2024

Current dependencies on/for this PR:

This comment was autogenerated by Freephite.

@cbrewster cbrewster force-pushed the 10-08-Add_backpressure_to_send_buffer branch from 2a05749 to bdfe692 Compare October 8, 2024 18:55
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heck yeah!

replit_river/message_buffer.py Show resolved Hide resolved
tests/test_message_buffer.py Outdated Show resolved Hide resolved
replit_river/message_buffer.py Show resolved Hide resolved
@cbrewster cbrewster force-pushed the 10-08-Add_backpressure_to_send_buffer branch from bdfe692 to 910dd0e Compare October 8, 2024 19:14
replit_river/session.py Outdated Show resolved Hide resolved
@cbrewster cbrewster force-pushed the 10-08-Add_backpressure_to_send_buffer branch from 910dd0e to 6ef4509 Compare October 8, 2024 19:18
@cbrewster cbrewster merged commit 80e6d95 into main Oct 8, 2024
3 checks passed
@cbrewster cbrewster deleted the 10-08-Add_backpressure_to_send_buffer branch October 8, 2024 19:21
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.

2 participants