river icon indicating copy to clipboard operation
river copied to clipboard

Enforce maximum payload size

Open cbrewster opened this issue 1 year ago • 1 comments

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 avatar Oct 24 '24 06:10 cbrewster

Current dependencies on/for this PR:

  • main:
    • PR #278
      • PR #279 👈

This comment was autogenerated by Freephite.

cbrewster avatar Oct 24 '24 06:10 cbrewster