5 ms·

On incomplete HTTP reads and the requests library in Python

92 points by s3rvac 8y ago

cryptonector 8y ago
I've written an HTTP service that offers "tail -f" as a service. When you GET a resource with a Range: bytes=0- (or some other starting offset with unspecified end offset) you get chunked transfer-encoding that doesn't terminate until the file is unlinked or renamed out of the way. This is incredibly handy, both for the usual things one might want to tail -f (log files), and as a cheap-but-very-functional pub/sub system. One thing I've learned is to watch out for silly proxies (client or reverse) that want to read the whole thing and then re-serve it as definite-length (i.e., not chunked, with Content-Length) or which impose a timeout on the transfer. HTTP needs a way to say that a chunked encoding is of indefinite length, though arguably the Range: header in my case ought to be all the hint the proxies (and libraries!) need.
- batbomb 8y ago
  Isn’t the chunk the unit, why should it be unbounded m? You’d have issues on the client side without knowing how long a chunk is going to be - It seems like the chunk should be the unit which can be guaranteed to fit in memory on the client.
  - cryptonector 8y ago
    Chunked transfer encoding means that the sender sends <length>\r\n<chunk-of-that-length>\r\n... but each chunk can be of different length. A zero-length chunk terminates the transfer. Chunked transfer encoding is used when the sender doesn't yet know the length of the resource's representation.
- zie 8y ago
  Is this OSS? Is it available for use anywhere?
  - cryptonector 8y ago
    Not yet. I've asked permission to open source it. It's basically epoll-based, evented, and it's darned simple.
- greglindahl 8y ago
  You could consider using websockets, which are intended to be used the way you're trying to use chunking.
  - cryptonector 8y ago
    I'm aware. But this is easier to use. For example, here's how you tail some log file: curl -H 'Range: bytes=0-' https://foo.example/bar.log https://foo.example/bar.log.
llccbb 8y ago
I am confused about the test used here. The `requests` docs very plainly state that `Response.ok` only checks the status code. Looking into the codebase proves that as well. Is there a status code for "I am going to send bad-length content"? This might be a UX problem and not a technical one. The author seems to think that `Response.ok` should be an all-purpose check.
- setr 8y ago
  Its completely a UX issue, but it seems reasonable to me. I agree itS not surprising behavior given the docs, but I believe it should be added. Its been a while since i used Requests, but iirc response.ok is basically syntax sugar; but it seems to me that in most valid usecases where you'd like this sugar (over being explicit in your actions), is when you'd like to verify the communications was correct. And malformed http is not correct. I imagine if you implemented a wrapper ok2, of correctness check + response.ok, you'd see 90% of response.ok become ok2 It seems to me to be a sensible check (validate that the http message meets the standard), that should exist in any http library at request's level. And response.ok seems like a wasted api slot, if its not meeting the full needs of its sugaring
- Twirrim 8y ago
  Honestly? I'm with the author here. "Response.ok" should tell me that the response is ok. It's the most natural expectation. If it's only going to check status code, Response.status_code.ok would be more reasonable.
  - llccbb 8y ago
    I agree with your point that there should be some baked in capability for a more comprehensive check on the HTTP response. There are probably some edge cases around this regarding stream=True (load header only, not content) and iter_content() which pulls chunks from a buffer so the entire content isn't loaded into memory at once. Probably surmountable. FWIW Response.status_code is an int, so it might need to be a member function on Response.
    - 8y ago
dfox 8y ago
As for the compression and resulting discrepancy of length: if you are doing transparent HTTP compression, you are supposed to send Transfer-Encoding: gzip (which implies chunked mode and that Content-Length shoul not be present and should be ignored by UA). Content-Encoding: gzip means that the resource itself is gzip compressed (which means that the UA should decompress it for displaying to user, but should save it to disk compressed, IE and Safari gets this wrong)
- tonyarkles 8y ago
  Ahhhhh... Thanks! I've always wondered why occasionally .tar.gz downloads end up ungzipped on disk (as just a .tar). Guess it was when using IE or Safari.
dagenix 8y ago
This is one of the most frustrating issues I've had with the requests/urllib3 libraries. I have no idea what I'm supposed to do with half of an HTML page or a PDF or a PNG. And even if it did, I certainly don't know how I'm supposed to handle those things when I don't get an indication that I'm likely dealing with an incomplete response. I can construct a use case where I might want this behavior (maybe a FLIF image), but it feels like such a huge outlier to me, that this being the default behavior of requests, which caters to making the common cases easy, has always felt a bit odd. I don't begrudge the authors for this at all. After all, these are people donating their time to work on libraries solving hard problems for free that I can then use as part of my job. There is nothing that has stopped me from forking these libraries and changing the behavior myself, other than a lack of time and that, while I dislike this behavior, its only is a problem somewhat infrequently. All that being said, I really wish the response to this issue has been something other than what feels like a language-lawerly reading of the spec (requests isn't an "agent" and so that sentence in the spec doesn't apply) and the theory that someone _might_ be able to do something with an incomplete response _some_ of the time and as such, the vastly more common case should be made much more complicated. But, anyway, I'm glad to hear that this issue is being addressed and I thank the authors for their work!
- greglindahl 8y ago
  Even after this is fixed the way you want it, webservers will still send you half of a PNG because it's a truncated file on the webserver. The webserver will give the truncated length as the length, the actual transfer will be the truncated length, requests will think all is well, and you'll have a problem.
  - dagenix 8y ago
    We can't fix every problem. And fixing every problem shouldn't be a gating function for fixing some problems. Web servers generally want to send a complete response. And sometimes some middle box chops off the response halfway through. And if that origin server set a correct Content-Length header and the response isn't that long, we can be fairly certain that the response is probably incomplete. And I really can't come up with a non-contrived use-case where we want those incomplete responses to be delivered silently to client code. It seems like what you are saying is that because a file might be corrupt we shouldn't worry about a completely unrelated case where the file is fine but the transfer is incomplete. By that logic, why do we worry about trying to report errors at all?
crdoconnor 8y ago
>Secondly, if we throw an exception we irrevocably destroy the data we read. It becomes impossible to access. This means that situations where the user might want to ‘muddle through’, taking as much of the data as they were able to read and keeping hold of it, becomes a little bit harder. This is an awful justification. They should do this by raising an exception and attaching the response data to it, so the user who wants to "muddle through" can catch the exception and "muddle through" explicitly. Regardless, this is a clear violation of https://en.wikipedia.org/wiki/Fail-fast https://en.wikipedia.org/wiki/Fail-fast and disappointing behavior from a package that prides itself on clear, obvious API behavior.
- cessor 8y ago
  I fully agree with you. Altough I really like requests, this is a violation of one of the PEP 20 heuristics: Errors should never pass silently. Unless explicitly silenced. https://www.python.org/dev/peps/pep-0020/ https://www.python.org/dev/peps/pep-0020/ An exception would be an elegant way to handle the problem and would be able to retain the incomplete data to be handeled. It's exactly what they are for. The exception could be the default to prevent surprises, possible deactivated with a flag.

92 points by s3rvac 8y ago

I've written an HTTP service that offers "tail -f" as a service. When you GET a resource with a Range: bytes=0- (or some other starting offset with unspecified end offset) you get chunked transfer-encoding that doesn't terminate until the file is unlinked or renamed out of the way. This is incredibly handy, both for the usual things one might want to tail -f (log files), and as a cheap-but-very-functional pub/sub system. One thing I've learned is to watch out for silly proxies (client or reverse) that want to read the whole thing and then re-serve it as definite-length (i.e., not chunked, with Content-Length) or which impose a timeout on the transfer. HTTP needs a way to say that a chunked encoding is of indefinite length, though arguably the Range: header in my case ought to be all the hint the proxies (and libraries!) need.

batbomb 8y ago
Isn’t the chunk the unit, why should it be unbounded m? You’d have issues on the client side without knowing how long a chunk is going to be - It seems like the chunk should be the unit which can be guaranteed to fit in memory on the client.
- cryptonector 8y ago
  Chunked transfer encoding means that the sender sends <length>\r\n<chunk-of-that-length>\r\n... but each chunk can be of different length. A zero-length chunk terminates the transfer. Chunked transfer encoding is used when the sender doesn't yet know the length of the resource's representation.
zie 8y ago
Is this OSS? Is it available for use anywhere?
- cryptonector 8y ago
  Not yet. I've asked permission to open source it. It's basically epoll-based, evented, and it's darned simple.
greglindahl 8y ago
You could consider using websockets, which are intended to be used the way you're trying to use chunking.
- cryptonector 8y ago
  I'm aware. But this is easier to use. For example, here's how you tail some log file: curl -H 'Range: bytes=0-' https://foo.example/bar.log https://foo.example/bar.log.

llccbb 8y ago

I am confused about the test used here. The `requests` docs very plainly state that `Response.ok` only checks the status code. Looking into the codebase proves that as well. Is there a status code for "I am going to send bad-length content"? This might be a UX problem and not a technical one. The author seems to think that `Response.ok` should be an all-purpose check.

setr 8y ago
Its completely a UX issue, but it seems reasonable to me. I agree itS not surprising behavior given the docs, but I believe it should be added. Its been a while since i used Requests, but iirc response.ok is basically syntax sugar; but it seems to me that in most valid usecases where you'd like this sugar (over being explicit in your actions), is when you'd like to verify the communications was correct. And malformed http is not correct. I imagine if you implemented a wrapper ok2, of correctness check + response.ok, you'd see 90% of response.ok become ok2 It seems to me to be a sensible check (validate that the http message meets the standard), that should exist in any http library at request's level. And response.ok seems like a wasted api slot, if its not meeting the full needs of its sugaring
Twirrim 8y ago
Honestly? I'm with the author here. "Response.ok" should tell me that the response is ok. It's the most natural expectation. If it's only going to check status code, Response.status_code.ok would be more reasonable.
- llccbb 8y ago
  I agree with your point that there should be some baked in capability for a more comprehensive check on the HTTP response. There are probably some edge cases around this regarding stream=True (load header only, not content) and iter_content() which pulls chunks from a buffer so the entire content isn't loaded into memory at once. Probably surmountable. FWIW Response.status_code is an int, so it might need to be a member function on Response.
  - 8y ago

dfox 8y ago

As for the compression and resulting discrepancy of length: if you are doing transparent HTTP compression, you are supposed to send Transfer-Encoding: gzip (which implies chunked mode and that Content-Length shoul not be present and should be ignored by UA). Content-Encoding: gzip means that the resource itself is gzip compressed (which means that the UA should decompress it for displaying to user, but should save it to disk compressed, IE and Safari gets this wrong)

tonyarkles 8y ago
Ahhhhh... Thanks! I've always wondered why occasionally .tar.gz downloads end up ungzipped on disk (as just a .tar). Guess it was when using IE or Safari.

dagenix 8y ago

This is one of the most frustrating issues I've had with the requests/urllib3 libraries. I have no idea what I'm supposed to do with half of an HTML page or a PDF or a PNG. And even if it did, I certainly don't know how I'm supposed to handle those things when I don't get an indication that I'm likely dealing with an incomplete response. I can construct a use case where I might want this behavior (maybe a FLIF image), but it feels like such a huge outlier to me, that this being the default behavior of requests, which caters to making the common cases easy, has always felt a bit odd. I don't begrudge the authors for this at all. After all, these are people donating their time to work on libraries solving hard problems for free that I can then use as part of my job. There is nothing that has stopped me from forking these libraries and changing the behavior myself, other than a lack of time and that, while I dislike this behavior, its only is a problem somewhat infrequently. All that being said, I really wish the response to this issue has been something other than what feels like a language-lawerly reading of the spec (requests isn't an "agent" and so that sentence in the spec doesn't apply) and the theory that someone _might_ be able to do something with an incomplete response _some_ of the time and as such, the vastly more common case should be made much more complicated. But, anyway, I'm glad to hear that this issue is being addressed and I thank the authors for their work!

greglindahl 8y ago
Even after this is fixed the way you want it, webservers will still send you half of a PNG because it's a truncated file on the webserver. The webserver will give the truncated length as the length, the actual transfer will be the truncated length, requests will think all is well, and you'll have a problem.
- dagenix 8y ago
  We can't fix every problem. And fixing every problem shouldn't be a gating function for fixing some problems. Web servers generally want to send a complete response. And sometimes some middle box chops off the response halfway through. And if that origin server set a correct Content-Length header and the response isn't that long, we can be fairly certain that the response is probably incomplete. And I really can't come up with a non-contrived use-case where we want those incomplete responses to be delivered silently to client code. It seems like what you are saying is that because a file might be corrupt we shouldn't worry about a completely unrelated case where the file is fine but the transfer is incomplete. By that logic, why do we worry about trying to report errors at all?

crdoconnor 8y ago

>Secondly, if we throw an exception we irrevocably destroy the data we read. It becomes impossible to access. This means that situations where the user might want to ‘muddle through’, taking as much of the data as they were able to read and keeping hold of it, becomes a little bit harder. This is an awful justification. They should do this by raising an exception and attaching the response data to it, so the user who wants to "muddle through" can catch the exception and "muddle through" explicitly. Regardless, this is a clear violation of https://en.wikipedia.org/wiki/Fail-fast https://en.wikipedia.org/wiki/Fail-fast and disappointing behavior from a package that prides itself on clear, obvious API behavior.

cessor 8y ago
I fully agree with you. Altough I really like requests, this is a violation of one of the PEP 20 heuristics: Errors should never pass silently. Unless explicitly silenced. https://www.python.org/dev/peps/pep-0020/ https://www.python.org/dev/peps/pep-0020/ An exception would be an elegant way to handle the problem and would be able to retain the incomplete data to be handeled. It's exactly what they are for. The exception could be the default to prevent surprises, possible deactivated with a flag.