-
-
Notifications
You must be signed in to change notification settings - Fork 847
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
HTTP/2 higher number of timeouts compared with 1.1 #841
Comments
Thanks, the results in the gist are very enlightening as to the magnitude of this issue... As you noted in #832, there might be an outstanding race condition or bug we fixed for HTTP/1.1 in the past but not HTTP/2. I guess we’ll have to dig into the HTTP/2 implementation to find out... If you’d like you can turn on TRACE logs and see if there’s anything odd there: HTTPX_LOG_LEVEL=trace python program.py |
@florimondmanca updated gist with results from running with |
It seems to get stuck here: |
Traces are too long and I can't upload files to gist, so I will upload them here: With HTTP/2 enabled -> trace.txt With HTTP/2 disabled -> trace_2.txt |
But I think the interesting bits are before and after the exceptions:
|
I tried to find something hanging, but no luck so far... |
I notice the KeyError’s are exactly 5s apart from one another. This is both the default connect timeout and the keepalive period for connections. So are they caused by our keepalive checking mechanism? Or is it a retry mechanism? Or is it what should have been a ConnectTimeout whose handling resulted in another error? |
Wait. 5 is also the number of requests fired off in a batch. I notice the snippet fires them off perfectly in parallel via Can you:
|
As for #817, does #817 (comment) suppress the KeyError for you as well? If so, what do TRACE logs look like with this patch? |
Tried with
|
With 3 requests in parallel:
|
added
|
Will try #817 fix next |
If I did this right:
The output was (without jitter):
|
With a timeout value of 10.0:
|
It appears that the time between errors is indeed the timeout time (read timeout) |
@florimondmanca @tomchristie excellent news here! I had 0 timeouts after retrying the tests here multiple times with the |
Fantastic! 👍 |
Just deployed a version of the service using the branch with |
Update: now with errors :/ both connect timeout and |
And now: Also: Also: |
@victoraugustolls Okay, so let's take these one at a time, in a bit more detail. They're generally looking roughly like valid behaviors that just need some neater error handling. I'd suggest starting by opening an issue on Once we've done that let's have open issues for each of these seperately on |
Closing as issues are reflected in httpcore encode/httpcore#45 and encode/httpcore#46. For the record, I just tested the original gist against httpx==0.13.0.dev2 and got the same result for http1.1 and http2. |
Nice! Will update on my end (and my application) to see the result! |
I recently observed that the number of timeouts with http2 enabled on Async client are higher than with it disabled. Made a gist with an example:
https://gist.github.com/victoraugustolls/01002ae218366b453e962446c9c8a274
The text was updated successfully, but these errors were encountered: