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 commentedon Feb 24, 2021
This may violate the RFCs. However, I'd need to check it.
ioquatix commentedon Feb 24, 2021
The VCR issue should be resolved by bblimke/webmock#930
ioquatix commentedon Feb 24, 2021
Regarding the issue here... I was able to reproduce it but I need to investigate further.
ioquatix commentedon Feb 24, 2021
I reproduced this backtrace:
ioquatix commentedon Feb 24, 2021
The response headers include transfer encoding which is invalid, but why is it included?
ioquatix commentedon Feb 24, 2021
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 commentedon Feb 24, 2021
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 commentedon Feb 24, 2021
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 commentedon Feb 24, 2021
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 likeconnection: 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 commentedon Feb 24, 2021
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 commentedon Feb 25, 2021
trevorturk commentedon Feb 25, 2021
Sounds good! Closing in favor of vcr/vcr#850