-
Notifications
You must be signed in to change notification settings - Fork 168
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
Since 2.6.0 ureq hangs for 5 seconds in recvfrom when making GET request to local OSRM server #600
Comments
Hi @Arvamer welcome to ureq! Thanks for the very detailed report! There were some changes in when we will return a connection to the agent pool. This could be involved. Sounds like it should be quite straightforward to verify the HTTP/1.0 theory with a unit test. I'll have a look soon! |
Indeed, thanks for the great report! When a response has no Content-Length or Transfer-Encoding, that means it is "close-delimited" - in other words, the response is done when the connection closes. If the server is holding the connection open for 5 seconds after sending a close-delimited response, I would expect ureq to keep trying to read that connection for 5 seconds. In the example, the server is sending So the question is - why would this have finished promptly in ureq 2.5.0? Here's the diff between the two. Since the response is JSON, in some sense it doesn't matter whether the server closes the connection promptly. If the client is willing to say "I saw the JSON close That approach is arguably incorrect. If the server sent some extra junk after the JSON finished, the client should get an error rather than ignoring the extra junk. There's a similar issue we happened to fix in 2.6.0: if the server sends extra junk after a gzip or Brotli response, we will read it and return an error (previously we ignored it and dropped the connection). I think we didn't fix it for JSON, and your example doesn't use So, I'm not sure yet exactly what the problem is, but I think the problem is in that general area. If you have time, it would be helpful to add
|
The 5s hang happens after "dropping stream" message. After hang the final part of response is printed, but looking at strace output it's probably some buffering because there is only a single
Actually original code was using ureq's |
Thanks for the additional logs!
Are you sure? The timestamps suggest the hang happens before the "dropping stream" message:
That would be compatible with the idea that ureq is waiting for the server to close the TCP connection, and the server is closing it after 5s. |
Tested again and you are right, it's printed after. |
Noticed that
Without gzip:
Or a bit better formatted content of received response from above strace (
|
@Arvamer sounds like there is some request header that triggers the behavior. Sometimes services treat |
Curl headers:
So looks same for me. Is it expected that content-length is not returned by |
Oh. That could be it. Maybe we ignore the content-length if it is HTTP/1.0? |
Yes, if the response was gzip-encoded and ureq automatically decompressed it (we do this to avoid confusion about the length of the decoded response).
Bingo! I think this may be the source of the problem: For an http/1.0 response, ureq considers the response body to be close-delimited, even if there was a content-length header. That's wrong. In fact, I have an old stale PR fixing it:
Thanks so much for providing a good real-world example motivating the fix. We'll get that cleaned up and submitted. |
Previously, we treated HTTP/1.0 responses as always being close-delimited. However, that's not quite right. HTTP/1.0 responses _default_ to Connection: close, but the server may send Connection: keep-alive, along with a Content-Length header. Update body_type to understand this. Also, slightly reorganize body_type. has_no_body was being checked redundantly when we could just early-return when has_no_body is true. And regardless of whether we see Connection: close (on any HTTP version), we must honor Transfer-Encoding or Content-Length if they are present. Change the handling of `Connection: close` to more directly affect whether a connection is pooled, regardless of what method was used to delimit the body. Fixes #600
Example code:
Response:
Strace:
Second
recvfrom
returns 0 bytes received after 5s.In 2.5, first
recvfrom
read less data, so second call torecvfrom
also returns something:OSRM from this docker image: https://hub.docker.com/r/osrm/osrm-backend/
My guess is on incorrect handling of missing content-length header/HTTP 1.0 .
The text was updated successfully, but these errors were encountered: