Enforce maximum payload size #279

cbrewster · 2024-10-24T06:26:52Z

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 · 2024-10-24T06:26:54Z

Current dependencies on/for this PR:

main:
- PR Encode messages before storing in send buffer #278
  - PR Enforce maximum payload size #279 👈

This comment was autogenerated by Freephite.

cbrewster · 2024-10-24T15:53:07Z

router/client.ts

+        // TODO: Is this the right error to send to the server?
+        sessionScopedSend(
+          cancelMessage(
+            streamId,
+            Err({
+              code: CANCEL_CODE,
+              message: 'cancelled by client',
+            }),
+          ),
+        );


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

cbrewster · 2024-10-24T15:54:03Z

router/client.ts

+          ),
+        );
+
+        throw e;


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.

cbrewster · 2024-10-24T15:55:21Z

transport/sessionStateMachine/common.ts

@@ -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,


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)

makes sense to me! we can probs do that follow up though, in our case we control all the clients so enforcing sender side is probably fine

jackyzha0 · 2024-12-17T17:35:14Z

PROTOCOL.md

+
+##### Server handling
+
+If the server attempts to send a message after a procedure has been started which exceeds the maximum payload size, the server must reply with a `MAX_PAYLOAD_SIZE_EXCEEDED` error and close down the procedure.


is there a reason we need a new protocol error? could we just use CANCEL and have max payload size exceeded in the message? the semantics of MAX_PAYLOAD_SIZE_EXCEEDED and CANCEL are identical in how they should be handled i think

Good point! My main goal was to make this error obvious when it happens, putting it in the message should be sufficient.

jackyzha0 · 2024-12-17T17:38:01Z

transport/sessionStateMachine/common.ts

@@ -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,


makes sense to me! we can probs do that follow up though, in our case we control all the clients so enforcing sender side is probably fine

jackyzha0 · 2024-12-17T17:54:10Z

router/client.ts

-      ? ControlFlags.StreamOpenBit | ControlFlags.StreamClosedBit
-      : ControlFlags.StreamOpenBit,
-  });
+  try {


we can probably handle this uniformly across server/client in

river/transport/transport.ts

Line 314 in 1007a55

getSessionBoundSendFn(

try catch the send and if it errors, send cancel instead

cbrewster mentioned this pull request Oct 24, 2024

Encode messages before storing in send buffer #278

Open

2 tasks

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

cbrewster commented Oct 24, 2024

View reviewed changes

cbrewster force-pushed the 10-24-Enforce_maximum_payload_size branch from 6a70fcc to 1f90cb9 Compare October 24, 2024 15:55

cbrewster marked this pull request as ready for review October 24, 2024 15:56

cbrewster requested a review from a team as a code owner October 24, 2024 15:56

cbrewster requested review from bradymadden97 and removed request for a team October 24, 2024 15:56

Enforce maximum payload size

0908b4d

cbrewster force-pushed the 10-24-Enforce_maximum_payload_size branch from 1f90cb9 to 0908b4d Compare October 24, 2024 16:13

lhchavez requested review from masad-frost and jackyzha0 October 24, 2024 17:07

bradymadden97 removed their request for review October 29, 2024 18:34

jackyzha0 reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce maximum payload size #279

Enforce maximum payload size #279

cbrewster commented Oct 24, 2024 •

edited

Loading

cbrewster commented Oct 24, 2024 •

edited

Loading

cbrewster Oct 24, 2024

cbrewster Oct 24, 2024

cbrewster Oct 24, 2024 •

edited

Loading

jackyzha0 Dec 17, 2024

jackyzha0 Dec 17, 2024

cbrewster Dec 17, 2024

jackyzha0 Dec 17, 2024

jackyzha0 Dec 17, 2024

jackyzha0 Dec 17, 2024


		##### Server handling

		If the server attempts to send a message after a procedure has been started which exceeds the maximum payload size, the server must reply with a `MAX_PAYLOAD_SIZE_EXCEEDED` error and close down the procedure.

Enforce maximum payload size #279

Are you sure you want to change the base?

Enforce maximum payload size #279

Conversation

cbrewster commented Oct 24, 2024 • edited Loading

Why

What changed

Versioning

cbrewster commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrewster Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrewster commented Oct 24, 2024 •

edited

Loading

cbrewster commented Oct 24, 2024 •

edited

Loading

cbrewster Oct 24, 2024 •

edited

Loading