request(..., preload_content=False).read(-1) raises an unexpected Exception

[kosta]

Subject

With preload_content=False, calling read(-1) on the body raises RuntimeError("buffer is empty") while read() or read(None) return the whole request body as expected.

Environment

import platform
import ssl
import urllib3

print("OS", platform.platform())
print("Python", platform.python_version())
print(ssl.OPENSSL_VERSION)
print("urllib3", urllib3.__version__)

prints:

OS macOS-13.5.1-arm64-arm-64bit
Python 3.11.4
OpenSSL 3.1.1 30 May 2023
urllib3 2.0.4

Steps to Reproduce

A simple and isolated way to reproduce the issue. A code snippet would be great.

Bad case:

import urllib3
http = urllib3.PoolManager()
http.request("GET", "https://httpbin.org/robots.txt", preload_content=False).read(-1)

raises: RuntimeError: buffer is empty.

while these work:

http.request("GET", "https://httpbin.org/robots.txt", preload_content=False).read()

or

http.request("GET", "https://httpbin.org/robots.txt", preload_content=False).read(None)

Expected Behavior

The whole buffer should be returned, as -1 is the expected default argument for fileobj according to the python stdlib.

Actual Behavior

it raises

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kosta/Library/Caches/pypoetry/virtualenvs/urllib3-read-IiCHxn1z-py3.11/lib/python3.11/site-packages/urllib3/response.py", line 877, in read
    return self._decoded_buffer.get(amt)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kosta/Library/Caches/pypoetry/virtualenvs/urllib3-read-IiCHxn1z-py3.11/lib/python3.11/site-packages/urllib3/response.py", line 255, in get
    raise RuntimeError("buffer is empty")

Note: urllib3 1.x raises:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kosta/Library/Caches/pypoetry/virtualenvs/uai-snaky-path-TevYsdGm-py3.9/lib/python3.9/site-packages/urllib3/response.py", line 567, in read
    data = self._fp_read(amt) if not fp_closed else b""
  File "/Users/kosta/Library/Caches/pypoetry/virtualenvs/uai-snaky-path-TevYsdGm-py3.9/lib/python3.9/site-packages/urllib3/response.py", line 533, in _fp_read
    return self._fp.read(amt) if amt is not None else self._fp.read()
  File "/opt/homebrew/Cellar/python@3.9/3.9.17_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/http/client.py", line 462, in read
    b = bytearray(amt)
ValueError: negative count

[sethmlarson]

Our HTTPResponse object actually behaves a lot more like an io.BufferedIOBase instead of an io.RawIOBase object due to how .read() is implemented to always return either amt bytes or between 0 and amt-1 bytes when EOF is reached. Perhaps our parent class io.IOBase isn't correct on HTTPResponse?

We actually get our behavior with -1 from the standard library http.client.HTTPResponse which is what our implementation is based on. It doesn't look like -1 would work there either?

[illia-v]

@smason and I discussed the issue in #3186 too.

I believe we have to check amt in read and read1 and set it to None in case of a negative value. This will make the functions compliant with io.BufferedIOBase.read docs.

If the argument is omitted, None, or negative, data is read and returned until EOF is reached.

Also, the http.client.HTTPResponse behavior has to be a CPython's bug. This is tracked in python/cpython#112064.

[kosta]

Awesome! Thank you for fixing :)