Server redirect containing neither body nor content-length header can result in delays/timeouts

When `requests` v2.28.1 follows an HTTP redirect, it attempts to [consume the content of the server response](https://github.com/psf/requests/blob/4d394574f5555a8ddcc38f707e0c9f57f55d9a3b/requests/sessions.py#L185-L188), and then [closes the response](https://github.com/psf/requests/blob/4d394574f5555a8ddcc38f707e0c9f57f55d9a3b/requests/sessions.py#L195-L196), allowing the socket to be re-used for other requests.

When that occurs for a server that sends an HTTP redirect (say, HTTP 302) without a content body -- and also without a `Content-Length` header (seemingly an optional header, particularly for responses that do not contain a response body), then `requests` can get stuck -- and/or timeout if a timeout is configured -- attempting to [read the body contents](https://github.com/psf/requests/blob/4d394574f5555a8ddcc38f707e0c9f57f55d9a3b/requests/sessions.py#L186) (more specifically: when [iterating the body contents](https://github.com/psf/requests/blob/4d394574f5555a8ddcc38f707e0c9f57f55d9a3b/requests/models.py#L899)).

## Expected Result
I'm not an expert in this area, so I'm not completely certain.  I feel like it might be possible to detect the absence of a `content-length` header field during the redirect logic, and also perhaps attempt to read the content (to empty the socket buffer), but perhaps with an immediate-skip if the content-length is missing (meaning: the server didn't say there is any body content, and there doesn't seem to be any body content from a brief check, so let's consider the content consumed already).

## Actual Result
When a timeout is configured for the request, then that timeout will be reached.

If no timeout is configured, then the request may remain open indefinitely.

## Reproduction Steps

```python
# Sample code derived from: https://docs.python.org/3/library/http.server.html#http.server.SimpleHTTPRequestHandler and sphinx: https://github.com/sphinx-doc/sphinx.git
from contextlib import contextmanager
from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer
from threading import Thread

import requests


class RedirectHandler(SimpleHTTPRequestHandler):
    protocol_version = "HTTP/1.1"

    def do_GET(self):
        if self.path == "/origin":
            self.send_response(302, "Found")
            self.send_header("Location", "http://127.0.0.1:8000/destination")
            self.end_headers()

        if self.path == "/destination":
            self.send_response(200, "OK")
            self.end_headers()


class RedirectServer(Thread):
    def __init__(self):
        super().__init__()
        self.server = ThreadingHTTPServer(("127.0.0.1", 8000), RedirectHandler)

    def run(self):
        self.server.serve_forever(poll_interval=0.001)

    def close(self):
        self.server.shutdown()


@contextmanager
def redirect_server():
    server = RedirectServer()
    server.start()
    try:
        yield server
    finally:
        server.close()


with redirect_server():
    requests.get(url="http://127.0.0.1:8000/origin", allow_redirects=True, timeout=1)
```

## System Information

    $ python -m requests.help

```json
{
  "chardet": {
    "version": "5.1.0"
  },
  "charset_normalizer": {
    "version": "3.0.1"
  },
  "cryptography": {
    "version": ""
  },
  "idna": {
    "version": "3.3"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.11.2"
  },
  "platform": {
    "release": "6.1.0-7-amd64",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "",
    "version": null
  },
  "requests": {
    "version": "2.28.1"
  },
  "system_ssl": {
    "version": "30000080"
  },
  "urllib3": {
    "version": "1.26.12"
  },
  "using_charset_normalizer": false,
  "using_pyopenssl": false
}
```

Edit: use a `requests.Session` object in the sample code
Edit #2: undo the previous edit

Hi @jayaddison,

If the server doesn't send an appropriate Content-Length or Transfer-Encoding header in the response, it's difficult for clients to know when the response is complete.

What Requests is doing here is correct to spec. We will try to read until the server closes the connection. I don't think we want to broadly change this as it's likely to break existing cases.

Reference [RFC 7230](https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3) (This should also be reflected in 9110)

> 7. Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

I agree with @nateprewitt heartily. As such I'm inclined to close this

@nateprewitt isn't rule six the applicable one from RFC 7230 in this case?

> If this is a request message and [none of the above](https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3) are true, then
> the message body length is zero (no message body is present).

It's a response, not a request message.

Off-topic, so only if interested: do either of you have any idea why RFC-7230 might have added that distinction between requests and responses at the end of the section 3.3.3 checklist?

(I have a half-baked and probably incorrect theory, and haven't yet found any relevant history on the web.. so I might hold onto that idea for now)

Never mind - I've found the answer in RFC-7230's predecessor: [RFC-2616, section 4.4](https://www.rfc-editor.org/rfc/rfc2616#section-4.4).  The final clause in the corresponding checklist there is/was:

> By the server closing the connection. (Closing the connection cannot be used to indicate the end of a request body, since that would leave no possibility for the server to send back a response.)

Despite all that, if an HTTP/1.1 server provides neither a `Content-Length` response header, nor a `Transfer-Encoding` declaration, then it feels to me like it should be valid for a client to return the underlying socket/connection to the pool.

It could require a flag/attribute to indicate that the socket is pending-content-read-completion -- but completing those reads feels like it should be performed asynchronously or in a separate thread, allowing the client to follow the redirect without delay (at the slight cost of an additional held-open socket on the client if the server is acting strangely.  probably better than blocking the client completely, though).

Not sure I'll have the time or inclination to write a suitable test case and implementation, but if I do, this comment is the notes I'll refer back to (and probably find that I misunderstood something, anyway).

> then it feels to me like it should be valid for a client to return the underlying socket/connection to the pool.

(this approach would only be possible within a `requests.Session`-based context, I think.  I'll edit the code sample accordingly for future (self-)reference)

> Despite all that, if an HTTP/1.1 server provides neither a `Content-Length` response header, nor a `Transfer-Encoding` declaration, then it feels to me like it should be valid for a client to return the underlying socket/connection to the pool.

Semantically we can't be certain that's a reasonable assumption. There are lots of server implementations and no way of knowing what they think it means semantically. 

> It could require a flag/attribute to indicate that the socket is pending-content-read-completion -- but completing those reads feels like it should be performed asynchronously or in a separate thread, allowing the client to follow the redirect without delay (at the slight cost of an additional held-open socket on the client if the server is acting strangely.  probably better than blocking the client completely, though).

This library cannot safely use threading. The ways and systems on which people use it are too varied for us to be able to confidently pick a solution there. We could implement a clock timeout on requests otherwise if we could pick something that would easily work for everyone without issues. We can't pick one of multiprocessing or threading because neither are consistently available across implementation, OS, etc. 

> Not sure I'll have the time or inclination to write a suitable test case and implementation, but if I do, this comment is the notes I'll refer back to (and probably find that I misunderstood something, anyway).

I closed this as not planned meaning we have no intention of "fixing" it. Why do you want to send a pull request we won't accept? 


Thanks - yep, I'm familiar with the idea of disparate and fragmented server dialects, and I realize that it can be risky to integrate changes that could cause communication problems, or cause applications to develop an expectation that diverges from spec.

> Why do you want to send a pull request we won't accept?

I'm not sure whether I'll get around to it, but the reason would be to document some progress towards a goal that other people might find useful too (either in understanding the limitations of a fix, or to adopt locally for their own testing, etc).

Please unsubscribe from this thread if this seems spammy; I'm likely to keep waffling on for a while.

I'm wondering if it'd be reasonable for a client that is following a server-issued redirect _not_ to wait to consume response body content if the server has not indicated `Content-Length`.

We could do that by issuing a zero-timeout read on the client socket.  If there's content there, then we could continue to wait until we have a complete response (basically: it looks like the server sent _something_, so let's read it).  That wouldn't require any threading or async.

It's possible the zero-timeout read would return no data -- in that case, the situation's a bit more ambiguous.  Is it just chance that the last packet(s) received didn't contain any of a response body that the server is _going_ to send?  Would it be reasonable to close the client-side socket in that case?  (requiring a new connection sometimes; perhaps with an emitted low-severity warning message)

None of that would require a session-based context, because it wouldn't involve the way-more-complicated idea of returning the connection to the pool and using another one while the redirect request is blocking.

We'd shift some of the potential resource-wastage from the client to the server, and that's certainly a tradeoff.  The benefit would be that in (hopefully rare) cases where servers don't include the `Content-Length` on a redirect response, clients should be able to proceed faster.

> We could do that by issuing a zero-timeout read on the client socket. If there's content there, then we could continue to wait until we have a complete response (basically: it looks like the server sent something, so let's read it). That wouldn't require any threading or async.

A change of this kind is likely to require code that calls `urllib3`.  Some notes:

* There is an existing [`is_connection_dropped` utility function in `urllib3`](https://github.com/urllib3/urllib3/blob/95ca35211d23d8baf7646e1f60aa31e3650178a8/src/urllib3/util/connection.py#L11-L30) that performs a zero-timeout read using platform-specific socket handling
* The `HTTPResponse.read` implementation in `urllib3` has some [special-case handling](https://github.com/urllib3/urllib3/blob/95ca35211d23d8baf7646e1f60aa31e3650178a8/src/urllib3/response.py#L572-L593) that relates to [Python bug 16298](https://bugs.python.org/issue16298) around closing connections when reading empty HTTP responses

Those aren't guaranteed to be relevant, but there's a chance that the utility method could be reusable here, and the Python bug is worth bearing in mind.

> Please unsubscribe from this thread if this seems spammy

If you're concerned about seeming spammy, maybe don't use an issue tracker on a repository with I don't remember how many subscribers as a notebook for stream of consciousness. 

I'd like to suggest that `requests` could follow [RFC 9110 section 9.3.2](https://httpwg.org/specs/rfc9110.html#HEAD) and [RFC 9112 section 6.3, item 1](https://httpwg.org/specs/rfc9112.html#message.body.length)\* to avoid blocking unnecessarily when a response body is not anticipated from the server during redirect-following of HTTP HEAD requests, and/or when encountering HTTP 1xx, 204 and 304 status code responses.

Please let me know whether a change of that kind would be acceptable.

\* broadly compatible with [RFC 7230 section 3.3.3, item 1](https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3)

@jayaddison:
> Please let me know whether a change of that kind would be acceptable.

Never mind: no change is required, evidently:

By adjusting the `requests.get` to `requests.head`, and `do_GET` to `do_HEAD` in the repro code, it's apparent that `requests` _does not_ block when an HTTP HEAD request is used and is directed by the server to follow an HTTP 302 redirect.  This was confirmed using `requests==2.30.0` and `urllib3` versions `1.25.11`, `1.26.15` and `2.0.2` (and that version list is not to imply that I believe other versions are affected, simply that those are the versions I've checked).

So: the behaviour here is to-spec, both for the `GET` and `HEAD` requests.  I should have tested that before asking about a possible change.