Unexpected Protocol::HTTP1::BadRequest exception

[trevorturk]

I'm attempting to migrate an app from https://github.com/typhoeus/typhoeus to https://github.com/socketry/async-http and I have a collection of old https://github.com/vcr/vcr cassettes where some hit an Protocol::HTTP1::BadRequest exception when reused. (Most of the cassettes seem to work just fine, but a significant number hit this error.)

While investigating the issue, I noticed that the response contained one of the headers, but not both. When I removed the one header, the request succeeds.

I created a short example demonstrating the issue here: https://github.com/trevorturk/async-http-vcr

Note that there's an additional issue where VCR is appending to existing cassettes with each successful test run when I believe it shouldn't. I'll file an issue in that project, but I'm not sure where the problem is. (Perhaps VCR doesn't play well with async libraries yet?)

Apologies in advance if this issue should have been filed in async-http, and apologies again as I suspect I'm simply misusing the library. I haven't been able to sort out how to adapt the accept_encoding: "gzip" header option from Typhoeus, and it's possible that this is the root cause of my issue.

[ioquatix]

This may violate the RFCs. However, I'd need to check it.

[ioquatix]

The VCR issue should be resolved by bblimke/webmock#930

[ioquatix]

Regarding the issue here... I was able to reproduce it but I need to investigate further.

[ioquatix]

I reproduced this backtrace:

Test#test_bad_request:
Protocol::HTTP1::BadRequest: Message contains both transfer encoding and content length!
    /Users/samuel/.gem/ruby/3.0.0/gems/protocol-http1-0.13.2/lib/protocol/http1/connection.rb:484:in `read_body'
    /Users/samuel/.gem/ruby/3.0.0/gems/protocol-http1-0.13.2/lib/protocol/http1/connection.rb:452:in `read_response_body'
    /Users/samuel/.gem/ruby/3.0.0/gems/protocol-http1-0.13.2/lib/protocol/http1/connection.rb:209:in `read_response'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/protocol/http1/response.rb:31:in `read'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/protocol/http1/client.rb:75:in `call'
    /Users/samuel/.gem/ruby/3.0.0/gems/protocol-http-0.21.0/lib/protocol/http/request.rb:53:in `call'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/client.rb:143:in `make_response'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/client.rb:106:in `call'
    /Users/samuel/.gem/ruby/3.0.0/gems/webmock-3.11.2/lib/webmock/http_lib_adapters/async_http_client_adapter.rb:65:in `call'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/internet.rb:53:in `call'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-http-0.54.1/lib/async/http/internet.rb:70:in `block (2 levels) in <class:Internet>'
    /Users/samuel/Documents/socketry/async-http/examples/async-http-vcr/test.rb:32:in `block (2 levels) in test_bad_request'
    /Users/samuel/.gem/ruby/3.0.0/gems/async-1.28.9/lib/async/task.rb:265:in `block in make_fiber'

[ioquatix]

The response headers include transfer encoding which is invalid, but why is it included?

[ioquatix]

Okay, I found it in dark_sky_1.yml VCR fixtures.

If you remove this, and use the patched VCR, it works as expected.

[ioquatix]

I think ideally, if the response has transfer-encoding chunked, body: should be an array of chunks. Otherwise, how do you know what boundary for the chunks was expected?

[trevorturk]

Thank you very much for looking into this. I agree it seems like Dark Sky's response is invalid, and I'm happy to remove the errant Transfer-Encoding - chunked bit and move along.

That being said, if these old VCR cassettes represent real-world responses from a popular API, perhaps it's worth considering if it's possible to work around their issues?

I'll start by digging into my existing cassette library to see if it's only Dark Sky that was sending invalid responses. Then I can just fix my old cassettes, move along, and see if the issue reoccurs when I go into production. We're doing ~300k requests/day so I think we should run into the issue again quickly or not at all.

We can close this issue or leave it open. In either case I'll keep it on my todo list and report back when I learn more.

[ioquatix]

Because I had to normalise behaviour across HTTP/1, 1.1 and 2, I had to make some decisions about what headers constitute "user level" and "protocol level". A lot of behaviour of certain HTTP/1.1 headers is critical to the protocol itself i.e. transfer-encoding: chunked indicates a specific format and if the protocol does not read that header, the connection will break. Some other ones include things like connection: close etc. Part of my decision was based on what logic was integrated directly into the protocol in HTTP/2 - i.e. which headers were replaced by flags in the HTTP/2 frames or implicitly part of the structure of the frames.

So, my feeling is, there are certain headers which should not ever be visible to the user - ones that directly impact the protocol but have little/no impact to client/user code.

Therefore, the solution would be to remove those headers from VCR, and might I suggest that any "hop headers" are ignored by VCR entirely. cc @olleolleolle see hop-to-hop headers as a starting point https://tools.ietf.org/html/rfc2616#page-92

[trevorturk]

Very interesting re: hop-to-hop headers -- I hadn't seen that distinction before, but it makes sense. So, I'll be removing those headers from my VCR cassettes.

I believe this issue can be closed and perhaps may be a useful reference for future Googlers.

I wonder if we should start a new issue in VCR for further discussion w/r/t your suggestion -- let's see if @olleolleolle has an opinion.

Thanks again!

[olleolleolle]

Hi! Thanks for adding me to the discussion. Hop-to-hop headers. When reading that section, I thought "should I consider VCR a _*cache*_ in this regard?" An exploratory GitHub Issuse in VCR could be very good: a home to land any findings, even if it "only" results in good search results for people debugging. If we can make a code change, also good.

…

On Wed, Feb 24, 2021 at 9:47 PM Trevor Turk ***@***.***> wrote: Very interesting re: hop-to-hop headers -- I hadn't seen that distinction before, but it makes sense. So, I'll be removing those headers from my VCR cassettes. I believe this issue can be closed and perhaps may be a useful reference for future Googlers. I wonder if we should start a new issue in VCR for further discussion w/r/t your suggestion -- let's see if @olleolleolle <https://github.com/olleolleolle> has an opinion. Thanks again! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAABUZHABPXWRLNQP5HEOTTAVQWJANCNFSM4YDGVHGA> .

[trevorturk]

Sounds good! Closing in favor of vcr/vcr#850