Skip to content

Unexpected Protocol::HTTP1::BadRequest exception #11

Closed
@trevorturk

Description

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.

Activity

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

ioquatix commented on Feb 24, 2021

@ioquatix
Member

image

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

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

image

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

ioquatix

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

trevorturk commented on Feb 24, 2021

@trevorturk
Author

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

ioquatix commented on Feb 24, 2021

@ioquatix
Member

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

trevorturk commented on Feb 24, 2021

@trevorturk
Author

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

olleolleolle commented on Feb 25, 2021

@olleolleolle
Contributor
trevorturk

trevorturk commented on Feb 25, 2021

@trevorturk
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

      Participants

      @olleolleolle@trevorturk@ioquatix

      Issue actions

        Unexpected Protocol::HTTP1::BadRequest exception · Issue #11 · socketry/protocol-http1