Skip to content

Issue #85 - fixes for CoreSizeLimitHandler#86

Merged
copybara-service[bot] merged 2 commits intomainfrom
fixSizeLimitHandler
Dec 15, 2023
Merged

Issue #85 - fixes for CoreSizeLimitHandler#86
copybara-service[bot] merged 2 commits intomainfrom
fixSizeLimitHandler

Conversation

@lachlan-roberts
Copy link
Copy Markdown
Collaborator

Issue #85

CoreSizeLimitHandler was not resetting its read/written counts for each request.

While testing this fix I also discovered that because it is using an HttpStream.Wrapper it is bypassing the GzipHandler, so I have changed this to also wrap the Request and Response instead of relying on wrapping the stream.

Note this CoreSizeLimitHandler is only used for Jetty 12 code, the Jetty 9 one does not have these issues.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts self-assigned this Dec 13, 2023
@lachlan-roberts
Copy link
Copy Markdown
Collaborator Author

Please also wait for a review from @gregw before merging.

I will also open a PR to test and fix this in Jetty, and once that is released we can use the Jetty version of this handler.

@ludoch
Copy link
Copy Markdown
Collaborator

ludoch commented Dec 14, 2023

Should we still wait?
How can we add a test for this?

@lachlan-roberts
Copy link
Copy Markdown
Collaborator Author

@ludoch this was approved in jetty/jetty.project#11051, with some remaining changes requested for the javadoc. So I'd say we're good to merge here.

I have tested this manually by deploying to prod, and I have added an additional test in the Jetty PR for this.

But would be good to add tests here as well.
If you want you can merge now and I can do a separate PR with tests early next week.

@copybara-service copybara-service bot merged commit f410df1 into main Dec 15, 2023
@lachlan-roberts lachlan-roberts deleted the fixSizeLimitHandler branch December 19, 2023 09:29
srinjoyray pushed a commit that referenced this pull request Nov 12, 2024
PiperOrigin-RevId: 591117937
Change-Id: I903eb5ac1f2d28ac4c093331d367d94e8907dad5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants