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

client, server: implement configurable wire message size limits. #172

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

klihub
Copy link
Member

@klihub klihub commented Aug 26, 2024

This PR implements configurable limits for the maximum accepted message size of the wire protocol. The default limits can be overridden using the new WithClientWireMessageLimit() option for clients and WithServerWireMessageLimit() option for servers.

In particular this PR includes commits to

  • implement the new options for setting client and server side message size limits
  • add new unit tests to exercise these options
  • update the protocol description to reflect the increased max. message size

@dmcgowan These options accept a maximum limit of 2^31-1 and a minimum limit of 4K. Therefore this PR effectively changes the protocol (as reflected in the updated protocol description) to use a reserved most significant bit in the frame data length field instead of a reserved most significant byte. Is this fine ? Or should we leave a few more bits reserved for future use, for instance by limiting the max. wire message size to 256M-1 leaving 4 bits unused ?

Updates:

  • max. message size reverted back to 16M
  • reverted defaults to no sender-side length check
  • added propagation of receiver-caught length errors back to sender

klihub and others added 2 commits August 21, 2024 17:52
Reject oversized messages on the sender side, keeping the
receiver side rejection intact. This should provide minimal
low-level plumbing for clients to attempt application level
corrective actions on the requestor side, if the high-level
protocol is designed with this in mind.

Co-authored-by: Alessio Cantillo <cantillo.trd@gmail.com>
Co-authored-by: Qian Zhang <cosmoer@qq.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Co-authored-by: Alessio Cantillo <cantillo.trd@gmail.com>
Co-authored-by: Qian Zhang <cosmoer@qq.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub requested a review from dmcgowan August 26, 2024 14:57
@klihub klihub force-pushed the devel/configurable-wire-message-size-limit branch from a59fcd6 to 454025b Compare August 27, 2024 07:25
Use a dedicated, grpc Status-compatible error to wrap the
unique grpc status code, the size of the rejected message
and the maximum allowed size when a message is rejected
due to size limitations by the sending side.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub changed the title Implement configurable wire message size limits. client, server: implement configurable wire message size limits. Aug 27, 2024
@klihub klihub requested a review from fuweid August 27, 2024 10:06
@klihub klihub force-pushed the devel/configurable-wire-message-size-limit branch from 454025b to fbe97d4 Compare August 27, 2024 10:20
@klihub klihub requested a review from mikebrow August 27, 2024 13:23
@dmcgowan
Copy link
Member

update the protocol description to reflect the increased max. message size

I wonder if this is necessary. The protocol allows for 16 MB but then we hardcode in the limit of 4 MB. I wonder if we can limit this change to make it configurable but keep the protocol max at 16 MB. We also don't seem to properly implement what the current language is, if the first byte is non-zero we should immediately fail with a protocol error rather than attempting to discard more than what the protocol allows.

While I'm sure there are use cases where more than 16 MB could be used, I'm not sure about just making this almost boundless on an object that is expected to be held and parsed in memory. Its better for the rpcs on top to implement their own streaming or framing.

Related to server/client agreeing. It is possible that we would want a client that maintains a 4 MB send since it may not know whether the server supports more, although allowing a larger response. Is that currently possible here? Could also be useful to allow getting the behavior it is now where the client does not even enforce the limit but just allows the server to fail. In that case it would be good to preserve the "max" in the error from the server.

@klihub
Copy link
Member Author

klihub commented Sep 11, 2024

update the protocol description to reflect the increased max. message size

I wonder if this is necessary. The protocol allows for 16 MB but then we hardcode in the limit of 4 MB. I wonder if we can limit this change to make it configurable but keep the protocol max at 16 MB. We also don't seem to properly implement what the current language is, if the first byte is non-zero we should immediately fail with a protocol error rather than attempting to discard more than what the protocol allows.

While I'm sure there are use cases where more than 16 MB could be used, I'm not sure about just making this almost boundless on an object that is expected to be held and parsed in memory. Its better for the rpcs on top to implement their own streaming or framing.

Good point. I can set the max. allowed message size to 16MB.

Related to server/client agreeing. It is possible that we would want a client that maintains a 4 MB send since it may not know whether the server supports more, although allowing a larger response. Is that currently possible here? Could also be useful to allow getting the behavior it is now where the client does not even enforce the limit but just allows the server to fail. In that case it would be good to preserve the "max" in the error from the server.

I can take a stab at it and see if I can rework the PR with these in mind. I think if we split the current max. allowed length to a max. allowed send and a max. allowed receive limit, plus update both the client and server side options for setting the send limit to interpret 0 as "no limit/don't check", then we could do all of the above. And we could probably also then restore the default behavior (no client or server options used) to the original/current one.

@klihub klihub force-pushed the devel/configurable-wire-message-size-limit branch 2 times, most recently from 6e89eda to 8c0eeb3 Compare September 12, 2024 18:44
@klihub
Copy link
Member Author

klihub commented Sep 12, 2024

@dmcgowan Here is my first stab at addressing the review comments:

This now

What is still missing are

  • allow disabling sender side length check to mimic original behavior
  • propagate receiver-reported max. message if the message was rejected by the receiver

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Fix a race where an asynchronous server.Serve() invoked in a
a goroutine races with an almost immediate server.Shutdown().
If Shutdown() finishes its locked closing of listeners before
Serve() gets around to add the new one, Serve will sit stuck
forever in l.Accept(), unless the caller closes the listener
in addition to Shutdown().

This is probably almost impossible to trigger in real life,
but some of the unit tests, which run the server and client
in the same process, occasionally do trigger this. Then, if
the test tries to verify a final ErrServerClosed error from
Serve() after Shutdown() it gets stuck forever.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub force-pushed the devel/configurable-wire-message-size-limit branch from 8c0eeb3 to e01a569 Compare September 13, 2024 13:59
Implement configurable limits for the maximum accepted message
size of the wire protocol. The default limit can be overridden
using the WithClientWireMessageLimit() option for clients and
using the WithServerWireMessageLimit() option for servers. Add
exported constants for the minimum, maximum and default limits.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Adjust unit test to accomodate for altered internal interfaces.
Add unit tests to exercise the new message size limit options.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub force-pushed the devel/configurable-wire-message-size-limit branch from e01a569 to 9e19b2d Compare September 13, 2024 14:08
@klihub
Copy link
Member Author

klihub commented Sep 13, 2024

@dmcgowan Here is my first stab at addressing the review comments:

This now

What is still missing are

  • allow disabling sender side length check to mimic original behavior
  • propagate receiver-reported max. message if the message was rejected by the receiver

@dmcgowan Updated to

  • add propagation of receiver-reported message size errors back to client, and
  • revert default behavior (with no limits set) to disable sender-side size checks

I'm not sure if disabled sender-side size check is still the default behavior we want, or if we'd rather just want a programmatic way to explicitly restore the old behavior. If it is the latter, I think we could do that by providing extra ReceiveOnlySizeCheck(size int) {ClientOpts,ServerOpt} and small changes to clampWireMessageLimit() and channel.maxMsgLimit(). Just let me know which is your preference and I can try making these changes if necessary.

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