close request connection if h11 sets client state as MUST_CLOSE

[theyashl]

Summary

Fixes: Issue- #2238

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[theyashl]

Please let me know where the documentation is required to be updated. Thanks!

[theyashl]

Hi @Kludex , can you please review this PR

[Kludex]

This test passes without the changes on this PR. Can we have a better test?

Is it the first request that doesn't send the response, or the second? 🤔

[theyashl]

Ummm... well the test case is failing if I remove the change made with this PR.

Below are the results:

• with fix:

scripts/test tests/protocols/test_http.py -k test_close_connection_with_multiple_requests
tests/protocols/test_http.py ..                                                                                                        [100%]

===================================================== 2 passed, 118 deselected in 0.11s ======================================================

• without fix (commented the fix from uvicorn/protocols/http/h11_impl.py file):

scripts/test tests/protocols/test_http.py -k test_close_connection_with_multiple_requests
tests/protocols/test_http.py .F                                                                                                        [100%]

================================================ 1 failed, 1 passed, 118 deselected in 0.31s =================================================

The error raised by failed test case:

E           h11._util.LocalProtocolError: can't handle event type Response when role=SERVER and state=MUST_CLOSE

After the fix, the second request should not send the response, since the first request has the Connection: close header.

[theyashl]

Removed newly introduced variable REQUEST_AFTER_CONNECTION_CLOSE and merged the test case with pre-existing test case test_return_close_header.
Since both test cases were working around Connection close header, hence merged them.

[Kludex]

Ah, it was a mistake on my side. I see the test failing now.

Can we have the REQUEST_AFTER_CONNECTION_CLOSE back? I actually prefer them to be separate.

As more important: we are accessing private attributes from h11. Does this means that the problem is actually with h11? Is there something missing on h11 that we can't use in uvicorn? I would prefer to not use private attributes.

[theyashl]

Can we have the REQUEST_AFTER_CONNECTION_CLOSE back? I actually prefer them to be separate.

Sure, will keep them as separate test cases.

While for the fix done, which is overriding the private variable of another library, I agree that we should not be doing so. It was the quickest and simplest way to do the fix when I had explored the issue, sorry for that.

While talking about the culprit here, I think improvement is needed in h11. Not only specific to this issue but overall when the client is sending Connection: close header, h11 needs to have a special event for that. Right now we are relying on h11's NEED_DATA event to break out of a while loop where we are reading next event using h11 Connection's next_event method.

next_event method returns NEED_DATA when the _receive_buffer is empty. While, when the request contains the Connection: close header, h11 should not send the NEED_DATA event to read further data. It does not makes sense to send NEED_DATA event, even when client is saying that close the connection. There has to be a special event to denote that the connection can be closed if the client's state is MUST_CLOSE. We can then handle breaking the loop in uvicorn based on this new event type along with NEED_DATA.
So the events occurrence stack would be: Request -> Data -> EndOfMessage -> CLOSE_CONNECTION (new special event type)

Let me know what you think of this approach.

And for the current fix, we could just replace the entire self.conn._receive_buffer._data overriding logic with a break statement.

if self.conn.their_state == h11.MUST_CLOSE:
    break

[Kludex]

Great job. 👍

Thanks @theyashl :)

[bashkirtsevich]

Server is not responding after upgrade on 0.30.4 version.

curl http://0.0.0.0:8000 return none response.

---

UVicorn startup command:

gunicorn -k uvicorn.workers.UvicornWorker --bind 0.0.0.0:8000 mywebapp.asgi:application

asgy.py file:

import os

from channels.auth import AuthMiddlewareStack
from channels.routing import ProtocolTypeRouter
from channels.routing import URLRouter
from django.core.asgi import get_asgi_application
from django.urls import path

os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'settings.local')

django_asgi_app = get_asgi_application()

application = ProtocolTypeRouter({
    'http': django_asgi_app,
    'websocket': AuthMiddlewareStack(
        URLRouter([
            ...
        ])
    )
})

[theyashl]

curl http://0.0.0.0:8000 return none response.

Hi @bashkirtsevich , I am not able to reproduce this behavior.

When tested with django's default asgi.py file, 0.0.0.0:8000 returned me django home page.

Used command: gunicorn -k uvicorn.workers.UvicornWorker --bind 0.0.0.0:8000 testme.asgi:application

pip freeze:

asgiref==3.8.1
channels==4.1.0
click==8.1.7
Django==5.0.7
gunicorn==22.0.0
h11==0.14.0
packaging==24.1
sqlparse==0.5.1
typing_extensions==4.12.2
uvicorn==0.30.4

Default asgi.py file:

import os

from django.core.asgi import get_asgi_application

os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'testme.settings')

application = get_asgi_application()

Can you please share the server bootup logs for further debugging?

[plagree]

I have issues when upgrading as well.
My nextjs app which proxies calls to my Django backend fails to proxy requests and gives me the following result: Failed to proxy http://localhost:8080/api/health Error: socket hang up. No log in the uvicorn server. Strangely, direct calls to the API via requests.get('http://localhost:8080/api/health) works.
This is only new with uvicorn 0.30.4, I checked it's all good with 0.30.3

[theyashl]

Hi @plagree,
this looks like an issue with the keep-alive header as per my initial analysis (https://stackoverflow.com/a/27835115/7212543, https://stackoverflow.com/questions/29767005/why-am-i-getting-error-socket-hang-up-response)

Can you please share a reproducible sample of code using which I can debug this issue?

In the meantime, I'll try to execute following scenarios on my own:

• will create a proxy server and backend uvicorn server. Will relay a request from client via proxy server and will disconnect the client. With both keep-alive true and false headers.
• Set keep-alive to true with small timeout limit.
• Will try to execute multiple requests within single socket connection, while setting the keep-alive header to false.

Just for confirmation, please check if the request headers for backend server does not contains Connection: close header. :)

[theyashl]

Did some analysis around this side effect caused by the fix present in 0.30.4

Steps to reproduce the issue:

• create a simple uvicorn server:

import uvicorn

async def app(scope, receive, send):
    assert scope['type'] == 'http'

    await send({
        'type': 'http.response.start',
        'status': 200,
        'headers': [
            (b'content-type', b'text/plain'),
        ],
    })
    await receive()
    await send({
        'type': 'http.response.body',
        'body': b'{"status": "ok"}',
    })

if __name__ == '__main__':
    uvicorn.run("example:app", port=8080, http='h11')

• hit an endpoint on the server:

printf 'GET / HTTP/1.1\r\nConnection: close\r\nHost: a\r\n\r\n' | nc localhost 8080

This will cause the request to go into a waiting state unless and until the client closes the connection.

Reason: When H11 completely processes the client's request, the event is of type EndOfMessage along with the client state set to MUST_CLOSE because of the Connection: close header. With 0.30.4, we are initiating a process to close the connection with the client while missed to update the RequestResponseCycle object's more_body field and setting the message_event field.
Because of this receive method of RequestResponseCycle goes into a waiting state by creating a Future object attached to the running loop. Upon the client's disconnection the server app resumes writing the response body but since the connection has been disconnected the further execution is stopped.

Will raise an PR with the fix.
@Kludex do we have to create different issue for this?

[bashkirtsevich]

Hi @theyashl

There is no differences of boot logs between 0.30.3 and 0.30.4.

Pay attention to websocket app in my case.

Can you make you patch switching on by an environment variable or CLI param; and disable it as a default?

[theyashl]

Hi @bashkirtsevich ,
The fix was done for closing the connection when Connection: close header is received. This is a documented behaviour for client-server communication as per https://datatracker.ietf.org/doc/html/rfc7230#section-6.6.

While, in present case, the server is unable to send response when Connection: close is present, which I have fixed on #2408. I hope this fix will solve your use case.

You can explicitly install the dev fix and try if the fix solves your issue or not. pip install git+https://github.com/theyashl/[email protected]