Fix ensure http connection is closed

[pacoyang]

Ref Issues: #1199 #1226
Ref PRs: #1192 #1244 #1307

Description

As #869 (released in 0.14.0) is missing handle the scenario of b"", it is possible to make the HTTP connection not close correctly.
Here is the memory usage plot with the current main branch, running a single tcping process for 5 minutes.

The memory usage is increasing, and the netstat shows many CLOSE_WAIT connections.

I read the code in #869 , I have found two issues:

1. As we handle the connection close or not in the protocol, I think we should pass the scenario of b"" to protocol.data_received too. It is up to the protocol to determine if the connection should be closed or not.
2. Reading the data from _buffer seems not a good way. In some scenarios, the client will initially send empty TCP pings, then _buffer will receive b"", determine if the connections should be closed or not through the _buffer data is confusing (refs from the read function).

Here is current PR code running a single tcping process for 5 minutes in the same testing environment:

Test codes:

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 send(
        {
            "type": "http.response.body",
            "body": b"Hello, world!",
        }
    )


if __name__ == "__main__":
    uvicorn.run(app, host="0.0.0.0", reload=False, log_level=5)

[Kludex]

Thanks for the PR @pacoyang!

I've confirmed that this PR solves our issues: memory leak and CLOSE_WAIT. The logic here also makes sense to me, and I've specially liked the usage of the MUST_CLOSE state. 👍

That being said... I still think uvicorn should revert #869 . It's been almost a year and no one took the initiative to continue the work into trio compatibility.

In any case, I'll wait 10 days. If no other member gets involved on this PR, I'll approve, merge and release.

[Kludex]

Fine 👍

[Kludex]

Fine 👍

[Kludex]

@tomchristie @abersheeran @euri10 @florimondmanca friendly ping if someone wants to check this as well

[pacoyang]

That being said... I still think uvicorn should revert #869 . It's been almost a year and no one took the initiative to continue the work into trio compatibility.

@Kludex Sure, I just want to solve this issue as soon as possible, any way is acceptable. I love uvicorn, but this serious problem has forced me to choose another ASGI server. It's been a while since the discovery.

Using a streams handler can do more things like move the keep-alive timeout task out of the protocol, so we don't need to write multiple times in the protocol implementation. The keep-alive timeout task trigger still has issue, when a TCP connection is established and the client does not send any bytes, the server does not close this connection when keep-alive timeout. It can be reproduced by telnet.

[bittermandel]

Just stumbled upon this when running on fly.io with TCP health checks. Luckily I could switch over to HTTP checks to get around this, but following this to be sure it's resolved.

[Kludex]

Thanks for the PR @pacoyang !