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 support for ReadMaxBytes option #311

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

pkwarren
Copy link
Contributor

@pkwarren pkwarren commented Jul 1, 2022

Update connect-go to allow configuring a ReadMaxBytes option. This is
useful to avoid excessive memory/cpu usage when dealing with very large
messages.

@pkwarren pkwarren requested a review from akshayjshah July 1, 2022 21:13
@pkwarren pkwarren force-pushed the pkw/read-max-bytes branch from f4d6b71 to aff2dcf Compare July 1, 2022 21:17
Update connect-go to allow configuring a ReadMaxBytes option. This is
useful to avoid excessive memory/cpu usage when dealing with very large
messages.
@pkwarren pkwarren force-pushed the pkw/read-max-bytes branch from 64ecc86 to d20f50a Compare July 5, 2022 23:35
@pkwarren pkwarren marked this pull request as ready for review July 5, 2022 23:35
@akshayjshah
Copy link
Member

Awesome, will try to review it tonight!

@mattrobenolt FYI this should land soon.

Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

This is excellent - I have literally nothing to add.

if err != nil {
return errorf(CodeInvalidArgument, "message is larger than configured max %d - unable to determine message size: %w", readMaxBytes, err)
}
return errorf(CodeInvalidArgument, "message size %d is larger than configured max %d", bytesRead+discardedBytes, readMaxBytes)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, love this! Good reason not to use discard.

t.Helper()
t.Run("equal_read_max", func(t *testing.T) {
t.Parallel()
// Serializes to exactly readMaxBytes (1024) - no errors expected
Copy link
Member

Choose a reason for hiding this comment

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

Excellent, glad you're testing this - looking at your code, I bet my previous implementation had an off-by-one error.

@akshayjshah akshayjshah merged commit 5f1c21e into main Jul 7, 2022
@akshayjshah akshayjshah deleted the pkw/read-max-bytes branch July 7, 2022 17:07
@akshayjshah
Copy link
Member

🤦🏽‍♂️ I didn't review this carefully enough. Per the gRPC specification, we should be using CodeResourceExhausted here. I'm working on a PR to fix it.

FYI, @mattrobenolt - there's a small change to this behavior incoming.

akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
Update connect-go to allow configuring a ReadMaxBytes option. This is
useful to avoid excessive memory/cpu usage when dealing with very large
messages.
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