Support read1() method in HTTPResponse

[smason]

Fix suggested in #2125 but I actually wanted to get #2123 fixed and this seems like a reasonable starting point.

Presumably needs more tests as I'm not doing much on that front at the moment. But didn't want to do too much before getting some feedback.

[illia-v]

Thanks for taking this on

[smason]

@illia-v thanks for the review!

As per the initial comment I wanted to get stream(None) returning sensible sized blocks, I presume I should do that in a separate pull-request when this one gets accepted?

[smason]

I've noticed that BufferedIOBase.read1 (and read) are specified to use size=-1 to indicate that "an arbitrary number of bytes are returned" whereas urllib3 uses None as the default value.

Is it worth changing this around at some point? that value gets propagated to the underlying stream and it feels correct to use the documented value for getting an arbitrary number of bytes.

[smason]

@illia-v I think I've addressed everything you noted, please let me know if I've missed anything!

[sethmlarson]

Thanks @smason! There's a lot happening in this one, would like to get my eyes on this one too.

[smason]

@sethmlarson when I started I thought it would be an easy change but with all the fiddly edge cases it became easier to do a bit of a refactor

let me know if it would help to split things out somehow, e.g. rework the commits into more logical chunks, or maybe a few smaller pull-requests?

[illia-v]

I've noticed that BufferedIOBase.read1 (and read) are specified to use size=-1 to indicate that "an arbitrary number of bytes are returned" whereas urllib3 uses None as the default value.

Is it worth changing this around at some point? that value gets propagated to the underlying stream and it feels correct to use the documented value for getting an arbitrary number of bytes.

It makes sense to treat -1 the same as None in read and read1. If this is done, setting -1 as a default shouldn't be a breaking change and may be accepted.
I can see you made changes toward this in _fp_read, but read still treats -1 and None differently when it decides whether to cache content, for example.
I'd suggest keeping the previous behavior of _fp_read when amt is -1, and moving the changes to a separate future PR.

let me know if it would help to split things out somehow, e.g. rework the commits into more logical chunks, or maybe a few smaller pull-requests?

Please split changes unnecessary for adding read1 (like refactoring of _fp_read, changes to read, and type hints in existing tests) into separate small PRs, it looks like some of them are more than refactoring so they may need more attention and separate changelog entries.

[illia-v]

As per the initial comment I wanted to get stream(None) returning sensible sized blocks, I presume I should do that in a separate pull-request when this one gets accepted?

Yes, please submit the change to stream separately.

If we use read1 during streaming, won't it just work to pass a sensible size of a block instead of None as amt?
BTW, amt of HTTPResponse.stream defaults to 65536.

[smason]

As per the initial comment I wanted to get stream(None) returning sensible sized blocks

If we use read1 during streaming, won't it just work to pass a sensible size of a block instead of None as amt? BTW, amt of HTTPResponse.stream defaults to 65536.

Yup, using read1 is what I wanted to do. Notably it would be able to yield byte sized chunks from endpoints like: https://httpbin.org/drip?duration=2&numbytes=8 which currently block until the stream closes before returning anything.

Having it default to 64KiB chunks would likely remain a good default; it's possible to get slightly higher performance with larger chunks but it's often not much beyond that point.

[smason]

I've noticed that BufferedIOBase.read1 (and read) are specified to use size=-1 to indicate that "an arbitrary number of bytes are returned" whereas urllib3 uses None as the default value.
Is it worth changing this around at some point? that value gets propagated to the underlying stream and it feels correct to use the documented value for getting an arbitrary number of bytes.

It makes sense to treat -1 the same as None in read and read1. If this is done, setting -1 as a default shouldn't be a breaking change and may be accepted. I can see you made changes toward this in _fp_read, but read still treats -1 and None differently when it decides whether to cache content, for example. I'd suggest keeping the previous behavior of _fp_read when amt is -1, and moving the changes to a separate future PR.

The conditionals were getting too complicated for me to have confidence in with the prior code. But thinking about it again it is a breaking change and hence it makes sense to keep this separate.

[illia-v]

Also, I forget to mention that I noticed that read1 makes a _raw_read call even if self._decoded_buffer contains some bytes.
BufferedIOBase.read1 documents that it reads with at most one call, so making 0 calls is allowed. What do you think about skipping _raw_read if len(self._decoded_buffer) >= amt or even always when the buffer is not empty?

[smason]

The conditionals were getting too complicated for me to have confidence in with the prior code. But thinking about it again it is a breaking change and hence it makes sense to keep this separate.

Been trying to do this sensibly, but _fp_read in HEAD:

urllib3/src/urllib3/response.py

Lines 783 to 793 in 4dd4a82

	while amt is None or amt != 0:
	if amt is not None:
	chunk_amt = min(amt, max_chunk_amt)
	amt -= chunk_amt
	else:
	chunk_amt = max_chunk_amt
	data = self._fp.read(chunk_amt)
	if not data:
	break
	buffer.write(data)
	del data # to reduce peak memory usage by `max_chunk_amt`.

looks broken for amt=-1. It'll just keep trying to read the entire stream ignoring the max_chunk_amt constraint.

Is that actually correct? maybe it would make sense to fix that as an initial pull-request? The aim would be to tidy up the handling of negative vs None amts.

[illia-v]

Been trying to do this sensibly, but _fp_read in HEAD looks broken for amt=-1

It just doesn't expect that -1 can be passed and neither read does. The response class implements io.IOBase which doesn't define a read method, so adding support for -1 is a new feature not a bug fix 😁

If possible, let's finish this firstly keeping the current read logic when -1 is passed and using the same for read1.

[illia-v]

It'll just keep trying to read the entire stream ignoring the max_chunk_amt constraint.

You can ignore max_chunk_amt for read1, but conditions related to c_int_max have to be taken into account.

[illia-v]

Actually, looking at the source code of CPython's http.client.HTTPResponse.read, -1 is not expected to be passed there too.

self._fp is usually an instance of http.client.HTTPResponse.

[smason]

Been trying to do this sensibly, but _fp_read in HEAD looks broken for amt=-1

It just doesn't expect that -1 can be passed and neither read does. The response class implements io.IOBase which doesn't define a read method, so adding support for -1 is a new feature not a bug fix 😁

Huh, good point, that makes things easier! I will ignore handling negative lengths for now as they seem broken in http.client.

Actually, looking at the source code of CPython's http.client.HTTPResponse.read, -1 is not expected to be passed there too.

Yup, passing -1 will read until the end of the stream, bypassing the check that amt <= self.length. That's assuming that http.client.HTTPResponse's fp is the expected BufferedReader that comes from a socket's makefile.

Can't find much in CPython's Github issues, about this. There are some mentions of the io API and negative values in python/cpython#67403 but nothing much closer. The following MWE just hangs for me:

con = http.client.HTTPConnection("httpbin.org")
con.request("GET", "/")
resp = con.getresponse()
assert resp.headers["connection"] == "keep-alive"
print(resp.read(-1))

which feels wrong, or am I missing something? Is it worth raising an issue about this?

[illia-v]

Can't find much in CPython's Github issues, about this. There are some mentions of the io API and negative values in python/cpython#67403 but nothing much closer. The following MWE just hangs for me:

con = http.client.HTTPConnection("httpbin.org")
con.request("GET", "/")
resp = con.getresponse()
assert resp.headers["connection"] == "keep-alive"
print(resp.read(-1))

which feels wrong, or am I missing something? Is it worth raising an issue about this?

The example prints HTML for me but with a significant delay, this feels wrong to me too.
http.client.HTTPResponse is said to implement io.BufferedIOBase since Python 3.5, so I'd expect it to treat None and a negative number as the read argument the same.

[smason]

which feels wrong, or am I missing something?

The example prints HTML for me but with a significant delay, this feels wrong to me too. http.client.HTTPResponse is said to implement io.BufferedIOBase since Python 3.5, so I'd expect it to treat None and a negative number as the read argument the same.

Glad it's not just me misinterpreting APIs! Have raised python/cpython#112064 about this.

[smason]

Please split changes unnecessary for adding read1 (like refactoring of _fp_read, changes to read, and type hints in existing tests) into separate small PRs, it looks like some of them are more than refactoring so they may need more attention and separate changelog entries.

I've squashed all my WIP changes into a single one that's just about adding read1 support.

Will create another pull-request for the cleanups I had in here.

[illia-v]

I've squashed all my WIP changes into a single one that's just about adding read1 support.

Thanks, this is really helpful!

[illia-v]

This looks pretty good with the latest changes!

I merged the main branch with some updates fixing CI timeouts. Could you please add more tests to fix the coverage misses?
Also, please add a test similar to test_requesting_large_resources_via_ssl for read1, and check the docs building failure in CI.

[illia-v]

Can this condition be simplified knowing that the decoded buffer is empty?

Suggested change

	if (not decode_content) or (not data and len(self._decoded_buffer) == 0):
	if not data:

[smason]

The decoder still needs to be flushed in this case doesn't it? i.e. not sure if it's valid to bypass the decode_content check, suggests I wasn't doing the right thing before

[illia-v]

It looks like read doesn't get to flushing the decoder when there is no data

[smason]

It looks like read doesn't get to flushing the decoder when there is no data

The code you linked to is in read1, are you referring to the original read method here?

I'm asking because I don't see any issue with my logic in read1. I've just noticed it still contains some redundancies due to me basing it on the read implemention (two very similar looking calls to self._decode() and self._decoded_buffer.put()), but I don't think that affects correctness.

[illia-v]

It looks like read doesn't get to flushing the decoder when there is no data

The code you linked to is in read1, are you referring to the original read method here?

Sorry for confusing you. I meant that the read method on the following lines returns an empty bytes object regardless of decode_content.

urllib3/src/urllib3/response.py

Lines 919 to 920 in bc41d52

	if not data and len(self._decoded_buffer) == 0:
	return data

It's like an early exit. So I assumed an if not data: return data condition will copy the behavior from read to read1. Do you see any issue in doing so?

[smason]

it looks like a bug to me...

git blame doesn't suggest anything obvious, c35033f is when read was the last changed and was quite a large change.

maybe there's a test for that specific behavior you know of?

[illia-v]

I commented out the mentioned read lines, and many tests failed because self._fp was None, so self._raw_read returned None, and then joining None with some bytes was attempted

It's possible to reproduce a similar error by running HTTPResponse().read1()

[smason]

Why does _raw_read return None?! the return type is just bytes!

Maybe it should raise an exception when the stream has been closed? Presume this is due to the stream getting implicitly closed when reading, but that feels as wrong. Probably a another separate issue.

I also note that the whole content decoding feels backwards to me, why does _init_decoder keep getting called on every read which then pulls the header string out every time. Surely that can be done once and then remembered — the headers shouldn't change should they?

[smason]

https://github.com/smason/urllib3/tree/tidy_decoder is an attempt at tidying up the decoder

[illia-v]

I agree that there is a room for tidying up this part of urllib3
Regarding _raw_read: returning None is a pity, maybe that's a consequence of self._fp defaulting to None in some cases like in tests, so that should be changed

[illia-v]

LGTM, just one small comment

@sethmlarson @pquentin do you want to take a look at the PR before we merge it?

[pquentin]

@illia-v I fully trust your judgment. Thanks!

[sethmlarson]

High-level question, how is read1() different than read()'s implementation except for passing read1=True into _raw_read()? If that is a potential implementation path (even if it's not as efficient due to not using get_all()) can that be considered?

I ask this because our HTTPResponse data reading is already so complicated, having two parallel and slightly different implementations makes me worried for future maintenance.

[sethmlarson]

nit: Let's use the method this parameter is serving as the name (ie read1=True?)

[sethmlarson]

I'm not sure why amt is None was added here? The new branch if one_read doesn't use amt.

[smason]

@sethmlarson read1 is a standard method provided by io.BufferedReaders which http.client.HTTPResponse also provides

this PR is mostly about getting read1 supported, there are various cleanups that make sense once it's in there. e.g. with compressed streams it doesn't make sense to me that amt is passed to read on the stream. the decompressed data might be much larger and hence using read1 would mean that the caller might get a response much earlier

I'm not sure why amt is None was added here? The new branch if one_read doesn't use amt.

without it this "slow path" would get used for large responses even if amt is small

[illia-v]

@sethmlarson in case urllib3 or http.client is going to read1 2 GiB or more and get the overflow error, the code under this if block prevents this by passing the maximum safe amount to read1

The condition would be more correct with the new amt is None check even without the read1 addition. Not having it had no adverse effect previously, but now it's either amt is None has to be added here or code on lines 804–805 has to become more complex.

[smason]

It occurred to me that read should be using read1 when a decoder is in use, i.e. doing something like _raw_read(65536, one_read=True) rather than just passing amt down the chain.
The user might have requested a specific number of bytes, but because of compression it likely won't take a read of that size from the socket to fulfill the request.

I'd expect a user to choose amt based on Content-Length that provides a size of an encoded (possibly compressed) body

Speaking as a user of requests I don't think I've ever done that! HTTPResponse.stream certainly doesn't do anything like that, so why would calling code (that presumably knows even less details about what's going on at the HTTP level) know what makes sense to pass here?

[smason]

I've squashed and rebased again, hopefully that will lead to a cleaner merge

[illia-v]

High-level question, how is read1() different than read()'s implementation except for passing read1=True into _raw_read()?

I can see two changes besides calling _raw_read:

• if there is at least one byte in self._decoded_buffer, read1 unlike read will always return the buffered content and make no _raw_read calls;
• read1 doesn't try to call _raw_read until amt is reached.

[pquentin]

I can see two changes besides calling _raw_read

Should this be in a code comment?

[smason]

I can see two changes besides calling _raw_read

Should this be in a code comment?

The docstring of read1 points to io.BufferedReader.read1 as guiding the semantics, i.e. performing at most one IO, and there's a comment near the top of read1 saying "try and respond without going to the network". I've tried to keep the code readable and saying much more would just seem to duplicate the code in comments, which is often counter productive in my experience.

I've tried to stay away from changing read as the changes were quickly getting out of hand.

[smason]

thanks for merging! 🎉