Ensure HTTP connections are closed in case of data-less TCP pings

[florimondmanca]

Closes #1226

Completes #869 (released in 0.14.0) with a missing branch: it is possible that data be b"", e.g. if the client is a health checker that uses empty TCP pings to verify a service is up. As shown in #1226, this may be the case in k8s environments for example. In that case, we should be mindful of closing the transport, i.e. closing the connection (hence the connection/memory leak reported in #1226).

To reproduce before/after, consider this server:

async def app(scope, receive, send):
    if scope["type"] == "lifespan":
        await lifespan(scope, receive, send)
        return

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

And this "ping as fast as you can" client:

import socket

while True:
    with socket.create_connection(("localhost", 8000)) as sock:
        pass

0. Fire up a resource monitor, e.g. Activity Monitor on macOS.
1. Launch the server.
2. Lookup the Python process activity.
3. Launch the client.

Before the change, the Python process memory would go up indefinitely, at a pace that varies from system to system. Eg I start from 16MB and get to >20MB and up in just a few seconds.

After the change, memory remains stable. Eg I see a stable 17.1 MB despite the client hitting the server with pings.

[florimondmanca]

Seems like there's more subtlety to it than I thought.

Dismiss

[euri10]

Suggested change

	if data == b"":
	self.on_response_complete()

[euri10]

seems like this pass tests and fix the issue if I'm correct

[euri10]

Suggested change

	protocol.data_received(data)
	if isinstance(protocol, H11Protocol):
	if data:
	protocol.data_received(data)
	elif isinstance(protocol, HttpToolsProtocol):
	protocol.data_received(data)

[florimondmanca]

I guess we should be moving that if data: check to H11Protocol instead, then.

[euri10]

the thing is that this b"" case is already handled by h11 in it's receive_data we use in our protocol's data_received

see https://gitlab.com/kalilinux/packages/python-h11/-/blob/kali/master/h11/_connection.py#L303-348

so I dont know

[euri10]

Suggested change

	elif event_type is h11.ConnectionClosed:
	if not self.transport.is_closing():
	self.transport.close()
	break

[Kludex]

Replaced by #1332