generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 181
fix(event-handler): threshold limit for compression not respected when content-length not set #4827
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
Merged
+145
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n content-length not set
|
sdangol
approved these changes
Dec 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
event-handler
This item relates to the Event Handler Utility
size/L
PRs between 100-499 LOC
tests
PRs that add or change tests
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.



Summary
This PR fixes the issue where if the content-length header is missing, requests will not be compressed by the
compressmiddleware. This fix was much trickier than expected. Initially I attempted to add it towebResponseToProxyResult, which is where the Web API response body is deserialized to a JSON string/base64 encoded string. I had hoped that this meant we would only have to read the body once to set set theContent-Lengthheader, however, this is too late because it is the last thing that runs when sending a response so all middleware has already executed.I did consider getting the content length in
handlerResultToWebResponse. This would run on every request and make the header avaialble to all middleware. However, I decided against this for two reasons, one it means every request body gets read twice, regardless of whether the user cares about the content-length header, secondly, because it makeshandlerResultToWebResponse.asynchronous. I am open to persuasion here though if others think this is a better way to go, perhaps we could make the content length behaviour opt-in:const app = new Router({setContentLength: true}).The choice I ended up going with was to do the content-length calculation in the
compressmiddleware itself, that way only users using that middleware will read the body twice. I have also added logic to disable this calculation for HTTP streaming responses.Changes
#resolveto take a new option identifying whether we are running in HTTP streaming mode. If we are, we set theTransfer-Ecodingheader tochunked.compressmiddleware to calculate thecontent-lengthif the header is not present and theTransfer-Ecodingheaderis not set tochunked..Issue number: closes #4751
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.