-
Notifications
You must be signed in to change notification settings - Fork 414
Fix HTTPX support (part 2) #943
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
base: master
Are you sure you want to change the base?
Conversation
c6a8d54
to
1179f25
Compare
@jairhenrique @hartwork could you review this? |
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.
Pull Request Overview
This PR fixes HTTPX support in VCR.py by changing the patch target from HTTPX's client layer to the lower-level httpcore transport layer, enabling compatibility with custom HTTPX transports like Hishel. The change addresses issues with async response stream handling and improves header encoding consistency.
- Replaces
httpx_stubs.py
withhttpcore_stubs.py
to patch at the transport layer - Updates patch target from
httpx.Client._send_single_request
tohttpcore.ConnectionPool.handle_request
- Fixes header encoding to use ASCII instead of UTF-8 per HTTP specification
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
vcr/stubs/httpx_stubs.py | Complete removal of HTTPX-level patching implementation |
vcr/stubs/httpcore_stubs.py | New httpcore-level implementation with proper stream handling and ASCII header encoding |
vcr/patch.py | Updates patch targets from httpx to httpcore and imports new stub functions |
setup.py | Adds httpcore as test dependency |
docs/installation.rst | Documents httpcore as supported HTTP library |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I have addressed the comments, let me know if anything else is needed. |
Hi @jairhenrique, is there anything I can do to help move this along? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #943 +/- ##
==========================================
+ Coverage 92.23% 92.34% +0.10%
==========================================
Files 27 27
Lines 1868 1854 -14
Branches 249 247 -2
==========================================
- Hits 1723 1712 -11
+ Misses 97 95 -2
+ Partials 48 47 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@seowalex LGTM @kevin1024 @hartwork can you help on this review? |
@jairhenrique sorry, no time for this atm. |
Motivation
VCR.py does not work with alternative transports such as Hishel (an HTTP caching library), due to 2 problems. Firstly, as noted by #941, VCR.py always performs a sync
read
, which conflicts with Hishel's usage of anAsyncIterable
in async requests. Secondly, even after the first problem is fixed, VCR.py still does not work properly with Hishel as the patch target is at a higher level than the HTTPX transport layer, causing the caching transport used by Hishel to be entirely bypassed.Background
HTTPX support was purported to be fixed by #784, but unfortunately some issues surrounding the use of custom HTTPX transports remain, as explained above. The root cause is that the patch target is wrong (which the #784 also tried to fix, claiming to use the "lowest, most atomic request for data over the wire"). However, I believe that the "lowest, most atomic request" is actually at the
httpcore
level, and not thehttpx
level.Summary of changes
Change patch target from
httpx.Client._send_single_request
/httpx.AsyncClient._send_single_request
tohttpcore.ConnectionPool.handle_request
/httpcore.AsyncConnectionPool.handle_async_request
#784 targeted
httpx.Client._send_single_request
/httpx.AsyncClient._send_single_request
, however, this PR targetshttpcore.ConnectionPool.handle_request
/httpcore.AsyncConnectionPool.handle_async_request
. Tracing the connection between the former and the latter shows that this is likely the more correct target:As demonstrated above, the previous patch target was at a level above the HTTPX transport layer (rendering any custom transports useless), but the new patch target is at a level below the HTTPX transport layer (allowing custom transports to work with VCR.py).
A natural question is why
httpcore.HTTPConnection.handle_request
/httpcore.AsyncHTTPConnection.handle_async_request
is not patched instead, given it is the lowest level inhttpcore
. I believe it is not a good candidate due to 2 reasons:Socks5Connection
,ForwardHTTPConnection
andTunnelHTTPConnection
, all of which would have to be patched.Encode/decode headers using ASCII
Per the HTTP specification, ASCII encoding should be used for headers, not UTF-8.
Properly consume the response stream
In #784, the response stream is retrieved using
read
/aread
. However, this is not correct, as HTTPX will consume the response stream iterator, causing VCR.py to hang indefinitely when trying to read from an empty iterator. To solve this, the response stream is reconstructed after being consumed. This should also solve the related problem in #895.Remove unnecessary history handling
Since we are now operating at a lower level, history no longer needs to be specially handled.
Clean up function parameters
Function parameters have been made as specific as possible, instead of relying on
*args
and**kwargs
.Tests
The proposed changes pass all current HTTPX tests.