-
Notifications
You must be signed in to change notification settings - Fork 0
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
remove Transfer-Encoding from Proxy responses #24
Conversation
response = requests.get(proxy.url) | ||
|
||
if chunked: | ||
assert response.headers["Transfer-Encoding"] == "chunked" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this assertion (checking that the encoding is chunked
) is somewhat testing the behavior of Werkzeug instead of the ProxyHandler itself, as it is the decision of Werkzeug to set the Transfer-Encoding
to chunked
if the data is a generator. I added the test more as documentation around the behavior and to put it to contrast the Proxy
behavior itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice catch and 🕵️ work! The added complexity isn't great, but understand why it's needed. Header manipulation has caused us a lot of issues in the past, so let's keep a close eye on any impact it may have!
Motivation
We've had an issue with the Mailhog extension when using HTTP/1.1 (https://github.com/localstack/localstack-extensions/blob/main/mailhog), where the content would get double
Transfer-Encoding: chunked
header.This was investigated, and we found out that Twisted would always automatically add the
Transfer-Encoding: chunked
header value if no content-length is set, even if there's already a value. Werkzeug has the same behavior, buthypercorn
does not.After investigation, PEP3333 (https://peps.python.org/pep-3333/#id34) actually says that a WSGI application should not set the header itself; as it is the responsibility of the web server to manage this. It is true that it felt wrong from the application itself to dictate the server behavior. I believe GZIP is a bit of a different beast, as it relates more to the content itself of the response than the way the server will send it back.
So the more natural fix is to remove the
Transfer-Encoding: chunked
value from the header returned by the proxy, as the webserver will take care of that. We have specific tests around the automatic adding of this header when the request does not have content-length and have a generator asresponse
.Changes
Transfer-Encoding
in its response headers to proxy forward, and that theProxyHandler
properly returns chunked responseTransfer-Encoding: chunked
value out ofTransfer-Encoding
so that we do not return it, but keepgzip
\cc @lukqw