Graceful handling of 204 responses that incorrectly a include non-zero Content-Length.

[aviramha]

Checklist

• The bug is reproducible against the latest release and/or master.
• There are no similar issues or pull requests to fix it yet.

Describe the bug

HTTPX Session encounters parsing error (RemoteProtocolError: malformed data) when an endpoint returns content when status code is 204 (HTTP No Content). Other Python http clients handle this scenario.. I'm not sure if httpx should actually handle this case, since this is a weird case, but maybe it's best to handle it?

To reproduce

setup FastAPI server with this code:

from typing import Optional

from fastapi import FastAPI
from starlette.status import HTTP_204_NO_CONTENT
app = FastAPI()


@app.get("/", status_code=HTTP_204_NO_CONTENT)
def read_root():
    print("d")

Run it using uvicorn. In another terminal run the following snippet:

import httpx
client = httpx.Client()
client.get('http://localhost:8000/")
# The second one will get an error
client.get('http://localhost:8000/")

Expected behavior

No errors?

Actual behavior

malformed data

Debugging material

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in get(self, url, params, headers, cookies, auth, allow_redirects, timeout)
    905         **Parameters**: See `httpx.request`.
    906         """
--> 907         return self.request(
    908             "GET",
    909             url,

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in request(self, method, url, content, data, files, json, params, headers, cookies, auth, allow_redirects, timeout)
    731             cookies=cookies,
    732         )
--> 733         return self.send(
    734             request, auth=auth, allow_redirects=allow_redirects, timeout=timeout
    735         )

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in send(self, request, stream, auth, allow_redirects, timeout)
    765         auth = self._build_request_auth(request, auth)
    766 
--> 767         response = self._send_handling_auth(
    768             request,
    769             auth=auth,

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in _send_handling_auth(self, request, auth, timeout, allow_redirects, history)
    803 
    804         while True:
--> 805             response = self._send_handling_redirects(
    806                 request,
    807                 timeout=timeout,

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in _send_handling_redirects(self, request, timeout, allow_redirects, history)
    835                 )
    836 
--> 837             response = self._send_single_request(request, timeout)
    838             response.history = list(history)
    839 

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_client.py in _send_single_request(self, request, timeout)
    859 
    860         with map_exceptions(HTTPCORE_EXC_MAP, request=request):
--> 861             (status_code, headers, stream, ext) = transport.request(
    862                 request.method.encode(),
    863                 request.url.raw,

~/.pyenv/versions/3.8.2/lib/python3.8/contextlib.py in __exit__(self, type, value, traceback)
    129                 value = type()
    130             try:
--> 131                 self.gen.throw(type, value, traceback)
    132             except StopIteration as exc:
    133                 # Suppress StopIteration *unless* it's the same exception that

~/.pyenv/versions/3.8.2/lib/python3.8/site-packages/httpx/_exceptions.py in map_exceptions(mapping, **kwargs)
    341 
    342         message = str(exc)
--> 343         raise mapped_exc(message, **kwargs) from exc  # type: ignore
    344 
    345 

RemoteProtocolError: malformed data

Environment

• OS: macOS
• Python version: 3.8.2
• HTTPX version: 0.16.1
• Async environment: Both (asyncio, blocking)
• HTTP proxy: no
• Custom certificates: no

[florimondmanca]

Hello @aviramha, that's a pretty interesting one, thanks.

I was able to reproduce against this script:

App:

async def app(scope, receive, send):
    await send(
        {
            "type": "http.response.start",
            "status": 204,
            "headers": [(b"content-length", b"4")],
        }
    )
    await send({"type": "http.response.body", "body": b"Hey!"})

Client:

import httpx

client = httpx.Client()

response = client.get("http://localhost:8000")
assert response.status_code == 204

client.get("http://localhost:8000")

Trace output:

TRACE [2021-02-18 16:49:18] httpx._config - load_ssl_context verify=True cert=None trust_env=True http2=False
TRACE [2021-02-18 16:49:18] httpx._config - load_verify_locations cafile=/Users/florimond.manca/Developer/encode-projects/httpx/venv/lib/python3.8/site-packages/certifi/cacert.pem
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - get_connection_from_pool=(b'http', b'localhost', 8000)
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - created connection=<SyncHTTPConnection http_version=UNKNOWN state=0>
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - adding connection to pool=<SyncHTTPConnection http_version=UNKNOWN state=0>
TRACE [2021-02-18 16:49:18] httpcore._sync.connection - open_socket origin=(b'http', b'localhost', 8000) timeout={'connect': 5.0, 'read': 5.0, 'write': 5.0, 'pool': 5.0}
TRACE [2021-02-18 16:49:18] httpcore._sync.connection - create_connection socket=<httpcore._backends.sync.SyncSocketStream object at 0x102f60040> http_version='HTTP/1.1'
TRACE [2021-02-18 16:49:18] httpcore._sync.connection - connection.request method=b'GET' url=(b'http', b'localhost', 8000, b'/') headers=[(b'Host', b'localhost:8000'), (b'Accept', b'*/*'), (b'Accept-Encoding', b'gzip, deflate, br'), (b'Connection', b'keep-alive'), (b'User-Agent', b'python-httpx/0.16.1')]
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - send_request method=b'GET' url=(b'http', b'localhost', 8000, b'/') headers=[(b'Host', b'localhost:8000'), (b'Accept', b'*/*'), (b'Accept-Encoding', b'gzip, deflate, br'), (b'Connection', b'keep-alive'), (b'User-Agent', b'python-httpx/0.16.1')]
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - send_data=Data(<0 bytes>)
DEBUG [2021-02-18 16:49:18] httpx._client - HTTP Request: GET http://localhost:8000 "HTTP/1.1 204 No Content"
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - receive_event=EndOfMessage(headers=[])
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - response_closed our_state=DONE their_state=DONE
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - get_connection_from_pool=(b'http', b'localhost', 8000)
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - reusing idle http11 connection=<SyncHTTPConnection http_version=HTTP/1.1 state=4>
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - reuse connection=<SyncHTTPConnection http_version=HTTP/1.1 state=1>
TRACE [2021-02-18 16:49:18] httpcore._sync.connection - connection.request method=b'GET' url=(b'http', b'localhost', 8000, b'/') headers=[(b'Host', b'localhost:8000'), (b'Accept', b'*/*'), (b'Accept-Encoding', b'gzip, deflate, br'), (b'Connection', b'keep-alive'), (b'User-Agent', b'python-httpx/0.16.1')]
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - send_request method=b'GET' url=(b'http', b'localhost', 8000, b'/') headers=[(b'Host', b'localhost:8000'), (b'Accept', b'*/*'), (b'Accept-Encoding', b'gzip, deflate, br'), (b'Connection', b'keep-alive'), (b'User-Agent', b'python-httpx/0.16.1')]
TRACE [2021-02-18 16:49:18] httpcore._sync.http11 - send_data=Data(<0 bytes>)
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - remove from pool connection=<SyncHTTPConnection http_version=HTTP/1.1 state=2>
TRACE [2021-02-18 16:49:18] httpcore._sync.connection_pool - removing connection from pool=<SyncHTTPConnection http_version=HTTP/1.1 state=2>

This does NOT reproduce if:

• We update app so that it returns no body.
• We send a single request (error occurs on 2nd request, as you mentioned).

The exception comes from a call to h11, here:

https://github.com/encode/httpcore/blob/12a25fdb311cc3f0feb2b7ca7f0afab46989fe19/httpcore/_sync/http11.py#L168-L169

The fact that it only occurs upon 2nd request makes me think that, despite calling conn.start_next_cycle(), h11 might have kept some internal state/buffer and processes that first, then it gets confused having received "malformed data".

But that's only a supposition. It's not clear to me whether we have a bug, or h11 does. I know that h11 is rather strict, so it's not really a surprise that it raises a RemoteProtocolError in this case (after all, the server is violating the HTTP protocol), but what surprises me is that it raises it on the second request-response cycle.

What could help would be to simulate sending the request through h11 only (see tutorial). 😃

[tomchristie]

HTTPX Session encounters parsing error (RemoteProtocolError: malformed data) when an endpoint returns content when status code is 204 (HTTP No Content).

Okay. So, not super surprising that h11 is correctly indicating this as a protocol violation, since it is...

The 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

And yes, h11 tends to be protocol strict rather than some more lenient parsers, simply by virtue of it having been so carefully and comprehensively designed. (thanks to Nathaniel J. Smith)

In terms of ecosystem here, what would be neater would be if either the server or the framework was indicating an error. (More likely the server, since the framework probably? shouldn't have that level of detail about the HTTP semantics built-in.)

Having said that, it's feasible that there's scope for more lenient "parse it if you can at all" for various edge cases in h11. (?) There's a couple of similar style issues on the tracker - eg. python-hyper/h11#113, or supporting 6xx and other invalid codes as briefly mentioned in python-hyper/h11#95.

[tomchristie]

Having switched from bug to u-x, it's actually slightly less clear to me, as it'd be preferable if the first request was the point at which this is handled. Perhaps there are gracefulnesses here such as force-closing the connection on responses that include invalid content bodies.

[aviramha]

I did some digging and also came with the h11 strictness. I opened a PR to fastapi to overcome this but maybe this should also be enforced via starlette?

[tomchristie]

Well, I'd probably suggest at the server level, rather than each individual framework. So perhaps in Uvicorn. It'd be interesting to know how Gunicorn handles this case.

[aviramha]

I wouldn't put my trust into Gunicorn as it's very lightweight and permissive. I did the test anyway which showed it allows the same issue to occur:

def app(environ, start_response):
    """Simplest possible application object"""
    data = b'Hello, World!\n'
    status = '204 No Content'
    response_headers = [
        ('Content-type', 'text/plain'),
        ('Content-Length', str(len(data)))
    ]
    start_response(status, response_headers)
    return iter([data])

curl output:

*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 9999 (#0)
> GET / HTTP/1.1
> Host: 127.0.0.1:9999
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 204 No Content
< Server: gunicorn/20.0.4
< Date: Fri, 19 Feb 2021 14:37:26 GMT
< Connection: close
< Content-type: text/plain
< Content-Length: 14
< 
* Excess found in a non pipelined read: excess = 14 url = / (zero-length body)
* Closing connection 0

[tomchristie]

Enforced by h11, here... https://github.com/python-hyper/h11/blob/4746b0a0fff341b70aaa4032b7fa49f613a1b586/h11/_connection.py#L73-L81

    # Step 1: some responses always have an empty body, regardless of what the
    # headers say.
    if type(event) is Response:
        if (
            event.status_code in (204, 304)
            or request_method == b"HEAD"
            or (request_method == b"CONNECT" and 200 <= event.status_code < 300)
        ):
            return ("content-length", (0,))

Would be useful to know if requests/urllib3 raises an error in this case on subsequent requests or not.

One sensible tactic here might be to close the connection if we get a 204/304/HEAD/Successful CONNECT that includes a non-zero Content-Length. That way we'll at least end up with graceful behaviour.

[dalazx]

Bumped into this issue too. The part of the first response's body that was not fully read is remaining in the underlying buffer. The second response reuses that buffer and hence is considered malformed.

illegal status line: bytearray(b'nullHTTP/1.1 201 Created')

In this ^ particular case the previous response was 204, but its body had null.
Clearly this is a bug and has nothing to do with servers not strictly conforming to the RFC.

[tomchristie]

Clearly this is a bug and has nothing to do with servers not strictly conforming to the RFC.

That would not be my conclusion - it looks very much like a valid behaviour in response to malformed server behaviour.
But I do think we should be robust to it, and handle it gracefully.

[dalazx]

@tomchristie thank you for your reply. let me share some more reasoning:

1. The HTTP 1.1 protocol is a request/response protocol. Unless httpx has a way of disabling HTTP pipelining explicitly, a response should be read in its entirety before sending the next request. In such a case the first response would be considered malformed, not the second one (how httpx currently handles it).
2. The issue was reproduced via AWS ALB (which supports pipelining of front-end connections). Imagine a situation where there is an ALB staying in front of hundreds of services and one of them mistakenly responds with 204, but with a few dangling bytes in its body. httpx would be constantly raising protocol exceptions on subsequent responses from unrelated services after getting a response from that malformed one.

[dalazx]

meanwhile a pretty-bad looking workaround:

        resp = self._client.patch(...)
        resp.read()
        # TODO: remove once https://github.com/encode/httpx/issues/1474 is
        # fixed
        resp.stream._stream._httpcore_stream.connection.close()

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tomchristie]

Still valid thx, @Stale bot.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[florimondmanca]

As far as the original example code is concerned here, FastAPI is at fault here (or at least, users returning None without specifying response_class=Response). I got bitten by it myself just now, and there are a number of open issues on the FastAPI issue tracker and elsewhere about this:

• Response content longer than Content-Length error for DELETE and NoContent fastapi/fastapi#4939
• How to send 204 response? fastapi/fastapi#717
• uvicorn's "Response content longer than Content-Length" RuntimeError for DELETE endpoints starlette#1635

[tiangolo]

Yep, FastAPI was at fault. This was solved in fastapi/fastapi#5145, available since FastAPI 0.79.0. 🎉