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

Enforce maximum payload size #279

Open
wants to merge 1 commit into
base: 10-23-Encode_messages_before_storing_in_send_buffer
Choose a base branch
from

Conversation

cbrewster
Copy link
Member

@cbrewster cbrewster commented Oct 24, 2024

Why

Memory is not unbounded and underlying transports have limits on maximum payload size. When we accidentally exceed the max payload size for an underlying transport, the connection can get torn down entirely. This is bad because it means one bad RPC call can bring down the entire connection.

Additionally, when things do explode, its hard to find the right error message that tells you that this was due to exceeding some payload limit.

Let's enforce a configurable max payload limit in river itself. It's up to users to make sure that this max payload size is <= the max payload size allowed by their transport layer.

Something to consider in the future is automatically chunking these messages in river; however, I think this is a bad idea because we want to avoid having large buffers in memory entirely. Having this restriction in river will force application developers to stream or chunk data and help avoid cases where loading large buffers into memory happens to work until we hit OOMs.

What changed

  • Add config value for setting the max payload size
    • Default is 4M to match gRPC
  • Add new built-in error code: MAX_PAYLOAD_SIZE_EXCEEDED
  • If we exceed the max payload size after encoding, throw and error
  • Handle these new errors accordingly in the client procedure handler
    • If init message: inject error in the readable only client side. Server never sees these.
    • If in the middle of a stream or upload: cancel the request on the server, inject error in the readable, throw error since write returns void
  • Handle these new errors accordingly in the server procedure handler
    • Return error with code MAX_PAYLOAD_SIZE_EXCEEDED whenever server attempts to send a payload that is too large
  • Updated the protocol docs to reflect this new error

Versioning

  • Breaking protocol change
  • Breaking ts/js API change

@cbrewster
Copy link
Member Author

cbrewster commented Oct 24, 2024

Current dependencies on/for this PR:

This comment was autogenerated by Freephite.

@cbrewster cbrewster force-pushed the 10-24-Enforce_maximum_payload_size branch 3 times, most recently from 7df315e to 6a70fcc Compare October 24, 2024 15:46
Comment on lines +325 to +334
// TODO: Is this the right error to send to the server?
sessionScopedSend(
cancelMessage(
streamId,
Err({
code: CANCEL_CODE,
message: 'cancelled by client',
}),
),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is the right message, to send, maybe it should send the payload size exceeded error to the server too? Currently the server only ever expects CANCEL_CODE here

),
);

throw e;
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems odd to have to throw here in addition to injecting the errors, but we do need a way for calls to write to error out if we hit this.

@@ -141,6 +141,15 @@ export interface SessionOptions {
* The codec to use for encoding/decoding messages over the wire
*/
codec: Codec;
/**
* The maximum payload size that is allowed to be sent to the peer.
* This does not enforce max payload side when receiving messages,
Copy link
Member Author

@cbrewster cbrewster Oct 24, 2024

Choose a reason for hiding this comment

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

Thoughts on enforcing this on received messages too? I think we get the most value out of this by just enforcing it on the sender side as long as we do that everywhere (server, client, python client)

@cbrewster cbrewster force-pushed the 10-24-Enforce_maximum_payload_size branch from 6a70fcc to 1f90cb9 Compare October 24, 2024 15:55
@cbrewster cbrewster marked this pull request as ready for review October 24, 2024 15:56
@cbrewster cbrewster requested a review from a team as a code owner October 24, 2024 15:56
@cbrewster cbrewster requested review from bradymadden97 and removed request for a team October 24, 2024 15:56
@cbrewster cbrewster force-pushed the 10-24-Enforce_maximum_payload_size branch from 1f90cb9 to 0908b4d Compare October 24, 2024 16:13
@bradymadden97 bradymadden97 removed their request for review October 29, 2024 18:34
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.

1 participant