http.client.HTTPResponse.read(-1) handled incorrectly

[smason]

Bug report

Bug description:

http.client.HTTPResponse doesn't handle negative reads the same as other readers, for example the following code will hang for a significant amount of time:

con = http.client.HTTPConnection("httpbin.org")
con.request("GET", "/get")
resp = con.getresponse()
# "connection: close" doesn't trigger this
assert resp.headers["connection"] == "keep-alive"
while chunk := resp.read(-1):
    print(chunk)

The negative parameter is passed onto the underlying socket which will cause it to try and read to the end-of-stream. For keep-alive connections this just blocks until the connection is closed by the server due to inactivity.

I think this is a bug with not checking for negative amt values in:

cpython/Lib/http/client.py

Lines 469 to 471 in 24216d0

	if self.length is not None and amt > self.length:
	# clip the read to the "end of response"
	amt = self.length

Changing the call to read1 causes the above to display the response promptly as I'd expect. This is due to it correctly checking for negative sizes.

cpython/Lib/http/client.py

Lines 654 to 655 in 24216d0

	if self.length is not None and (n < 0 or n > self.length):
	n = self.length

Note that in earlier Python versions, e.g. 3.9, the above fails with ValueError: negative count which seems better than timing out, but I think reading to the end of the response makes more sense.

CPython versions tested on:

3.11, 3.12

Operating systems tested on:

Linux

[illia-v]

@smason thanks for opening the issue!

I'll bring a bit more context from the urllib3/urllib3#3186 discussion here.
http.client.HTTPResponse is said to implement io.BufferedIOBase since Python 3.5. Then io.BufferedIOBase.read specifies that if its "argument is omitted, None, or negative, data is read and returned until EOF is reached".

This together leads to an assumption that the read method should treat any negative number the same as it treats None in the whole following block:

cpython/Lib/http/client.py

Lines 468 to 494 in 24216d0

	if amt is not None:
	if self.length is not None and amt > self.length:
	# clip the read to the "end of response"
	amt = self.length
	s = self.fp.read(amt)
	if not s and amt:
	# Ideally, we would raise IncompleteRead if the content-length
	# wasn't satisfied, but it might break compatibility.
	self._close_conn()
	elif self.length is not None:
	self.length -= len(s)
	if not self.length:
	self._close_conn()
	return s
	else:
	# Amount is not given (unbounded read) so we must check self.length
	if self.length is None:
	s = self.fp.read()
	else:
	try:
	s = self._safe_read(self.length)
	except IncompleteRead:
	self._close_conn()
	raise
	self.length = 0
	self._close_conn() # we read everything
	return s

[ljmc-github]

If no one is working on a PR, I’m happy to take this.

[smason]

I was waiting from somebody official to say whether this was a real issue before making a PR...

I;ve also noticed that http.client.HTTPResponse doesn't have read1 documented and was thinking of putting that in as well.