All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Should I create an issue/bug before that PR?

Should I create an issue/bug before that PR?

Yes.

Now, the prototype of HTTPResponse.read() is wrong and this is the one that should be fixed instead. If you look at the call chain, you'll see that passing amt=-1 actually leads to a lot of issues. For instance:

while (chunk_left := self._get_chunk_left()) is not None:
    if amt is not None and amt <= chunk_left:
        value.append(self._safe_read(amt))
        self.chunk_left = chunk_left - amt
        break

We have an issue with self.chunk_left = chunk_left - amt and self._safe_read. Instead, we should:

• assert that amt is None or amt >= 0 in private methods
• change amt = -1 to amt = None when calling read(). Namely read(-1) should be equivalent to read(None).

Btw, negative reading size means "read everything you can" (see: https://docs.python.org/3/library/io.html#io.BufferedIOBase.read).

I created issue: #128271

Considering the tests are failing and that the implementation should handle negative reading sizes as being equivalent to None, I'm converting this PR to a draft PR. When everything is corrected, you can ask for a review from me.

By the way, we need some tests for that. But focus on fixing the implementation first and I can help you for the tests later.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

• You can forget about hypothesis tests (the failure is known)

• You should add a NEWS entry via blurb. Something like:

  Fix incorrect handling of negative read sizes in :meth:`HTTPResponse.read
  <http.client.HTTPResponse.read>`. Patch by YOUR_NAME.

  If you don't want to, you can leave out "Patch by [...]".

• We can now test this. You can improve BasicTest.test_chunked in test_httplib.py by adding a test case for None and -1. You can take inspiration from

  for n in range(1, 12):
      sock = FakeSocket(chunked_start + last_chunk + chunked_end)
      resp = client.HTTPResponse(sock, method="GET")
      resp.begin()
      self.assertEqual(resp.read(n) + resp.read(n) + resp.read(), expected)
      resp.close()

  where you'll need to modify n and adapt the assertEqual assertion.

FYI, we've had a similar change done in urllib3 urllib3/urllib3#3356

Thanks for the information. We could also change our own _read1_chunked but the latter already handles negative n so... it should be fine on our side. However, @manushkin could you perhaps add some tests for read1(None) or read1(-123) as well (if there are none?) [you can also decide to make a follow-up PR for that]

Note: I didn't quite understand whether you confirmed or not my suggestion of keeping it _read_chunked().

I think it's probably better to keep it in _read_chunked. While making the check in read() would make the two versions (non-chunked and chunked) eventually equivalent most of the time, we would still be reading fp in terms of chunk and not in one go, doing multiple calls (I've extracted the relevant lines of _read_chunked):

while (chunk_left := self._get_chunk_left()) is not None:
    value.append(self._safe_read(chunk_left))

If the HTTP response is meant to be read by chunks, we should do it so since "read as many bytes you can in chunks" and "read as many bytes you can in one go" may not be equivalent (depending on how the server sends the data, right?)

Let me clarify my point 🙂

This PR makes read handle negative integers and None the same for the if self.chunked case.

But if the response is not chunked, there are still different code paths for negative ints and None. So to fix read fully, the read method needs to handle negative integers anyway, either like in the following diff or in its beginning to pass only positive integers or None to self._read_chunked.

diff --git a/Lib/http/client.py b/Lib/http/client.py
index 1c0332d82bd..33a858d34ae 100644                                                                                                                                                                                            
--- a/Lib/http/client.py                                                                                                                                                                                                         
+++ b/Lib/http/client.py                                                                                                                                                                                                         
@@ -472,7 +472,7 @@ def read(self, amt=None):                                                                                                                                                                                    
         if self.chunked:                                                                                                                                                                                                        
             return self._read_chunked(amt)                                                                                                                                                                                      
                                                                                                                                                                                                                                 
-        if amt is not None:                                                                                                                                                                                                     
+        if amt is not None and amt >= 0:                                                                                                                                                                                        
             if self.length is not None and amt > self.length:                                                                                                                                                                   
                 # clip the read to the "end of response"                                                                                                                                                                        
                 amt = self.length   

if amt is not None and amt >= 0:

This is one is not really needed because we would bypass amt > self.length and directly call self.fp.read(amt) which would delegate to BufferIOBase.read() which handles negative ones correctly. But I agree that it's probably better to use the None path.

if amt is not None and amt >= 0:

This is one is not really needed because we would bypass amt > self.length and directly call self.fp.read(amt) which would delegate to BufferIOBase.read() which handles negative ones correctly. But I agree that it's probably better to use the None path.

In most cases it's not needed indeed, but there is some logic related to raising IncompleteRead in the None path which is skipped for negative ints currently

Is there anything else expected of me?

Is there anything else expected of me?

No. At least not until a core dev has reviewed it this PR. Maybe @vadmium wants to have a look?

This looks okay to me

Thanks @manushkin for the PR, and @Eclips4 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

GH-129395 is a backport of this pull request to the 3.13 branch.

GH-129396 is a backport of this pull request to the 3.12 branch.