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

enable_compression causes exception on HTTP/1.0 #1828

Closed
hubo1016 opened this issue Apr 21, 2017 · 14 comments · Fixed by #1910
Closed

enable_compression causes exception on HTTP/1.0 #1828

hubo1016 opened this issue Apr 21, 2017 · 14 comments · Fixed by #1910
Labels

Comments

@hubo1016
Copy link
Contributor

Long story short

call set_compression() on a response object and return it to aiohttp server causes a RuntimeError ("Using chunked encoding is forbidden for HTTP/1.0")

Expected behaviour

There should be at least two ways to use compression on HTTP/1.0:

  1. Fully compress the content, and return the length of the compressed data in the Content-Length header;
  2. Do not return Content-Length header, return the compressed stream and set Connection: close. Close the connection after transmit.

Actual behaviour

An runtime error is raised, and the connection is closed immediately.

In fact, it seems that by default, even a HTTP/1.1 request is responded with a HTTP/1.0 response, so it always raises this exception.

Steps to reproduce

Call enable_compression on any response object before returning.

Comment

I think it is absolutely not correct to enable chunked encoding on compression. Content-Encoding is not a part of transfer, it is a part of the content. It should never affect the transfer encoding type.

@hubo1016
Copy link
Contributor Author

Sorry, the "HTTP/1.1 is responsed with HTTP/1.0" is caused by a forwarding nginx.

@hubo1016
Copy link
Contributor Author

A related problem also exists on the client side:
seems that the client always assume chunked encoding on "content-encoding: deflate" ?

@fafhrd91
Copy link
Member

we do not know final size of payload, so it is just simpler to switch to chunked encoding.
nothing prevents you from setting content-length and send compressed payload.

@fafhrd91
Copy link
Member

I am not interested in changing current behavior. we may use compression without chunking for simple web.Request but for streaming response it seems easier to allow developer to choose what he wants.

@hubo1016 how would you fix this?

@hubo1016
Copy link
Contributor Author

@fafhrd91 I think compress with "Content-Encoding" should not affect the transfer encoding, they are on different layer. What is the expected behavior when sending a stream data without a specified length on a HTTP/1.0 connection? (According to RFC, the server should send a "Connection: close" header and shutdown the connection write side after all payload are sent, read and drop extra messages from the client which may be pipelining requests, then close the connection.)

I will dig deeper when I have time. Maybe it is just simplier to create a speicifed "content" which is compressed, either as a fixed-length bytes or a stream. Then we use the exactly same tranfer technology to transfer the compressed payload acrossing the HTTP channel.

@fafhrd91
Copy link
Member

@hubo1016 would you like to work on this issue?

@fafhrd91
Copy link
Member

seems, nobody wants to work on this. I suggest to close.

@asvetlov ?

@asvetlov
Copy link
Member

The issue in not in my priority list but please postpone the closing.
We could implement palliative solution by raising an exception with comprehensive error message on enabling compression for HTTP 1.0 as an option (both server and client side).
Specifying explicit compression header with sending pre-compressed body is still available workaround.
But yes, I have no strong motivation for supporting HTTP 1.0 quirks.

@hubo1016
Copy link
Contributor Author

@fafhrd91 Finally I have time for this issue, but firstly I want to read the existing codes and evaluate the effort. I will have a conclusion in a week.

@hubo1016
Copy link
Contributor Author

@fafhrd91 @asvetlov I have read the related code. The fix is quite straight-forward: instead of set _chunked directly, just remove the Content-Length header instead. Also the content-length property in Response class must be re-implemented to return the length of the compressed data.

Still some more tests should be added for this new behavior, including tests with version set to HTTP/1.0.

There is also some related issues and I need your opinions:

  1. According to RFC (https://tools.ietf.org/html/rfc7230#section-4.2.2), a "deflate" encoding should be a zlib format deflate stream, but aiohttp currently generates "raw" stream (by setting wbits=-15), which is mentioned as "non-conformant implementations" in the RFC. Should we replace it with wbits=15 to generate a zlib-wrapped stream?
  2. Content decoding is currently deeply planted in the HttpParser, which might be a problem because Content-Encoding is actually an attribute of the content (same as Content-Type, different from Transfer-Encoding). The client should be able to keep the compressed data without decoding it when necessary. Also, if a partial part of the payload (Status 206) is requested with Range header (as mentioned in https://tools.ietf.org/html/rfc7233), the content cannot be decoded even if the content-encoding is "gzip" or "deflate". We might choose to keep the data untouched and decode them in Request or ClientResponse (or not if explictly set to "no auto-decoding"), but this decision could be hard to make.

I will submit the pull request after reparing unittests. The current status can be viewed in hubo1016#2

@mickeyl
Copy link

mickeyl commented May 24, 2017

@hubo1016 the "HTTP/1.1 is responsed with HTTP/1.0" is caused by a forwarding nginx. Why is that so and can it be fixed in the nginx config?

@hubo1016
Copy link
Contributor Author

@mickeyl our configuration did not set the http version, so it defaults to HTTP/1.0, it can be fixed by setting the http version. But I still think it should be fixed.

@mickeyl
Copy link

mickeyl commented May 24, 2017

Thanks, I have since found the neccessary proxy_http_version 1.1; statement. In my opinion, nginx is clearly validating the spec here w/ chunked encoding, but I agree, we don't live in a perfect world and should try to be compatible, if it doesn't destabilize the codebase for the normal cases.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants