Repository navigation
httptools: Connection: Close does not close the connection #3071
|
On the
This looks like a bug rather than intentional behavior:
VersionsReproduced on current Reproductionimport asyncio
from tests.protocols.test_http import get_connected_protocol
from tests.response import Response
from uvicorn.protocols.http.h11_impl import H11Protocol
from uvicorn.protocols.http.httptools_impl import HttpToolsProtocol
from uvicorn.protocols.http.zttp_impl import ZttpProtocol
REQUESTS = {
"lower": b"GET / HTTP/1.1\r\nHost: example.org\r\nConnection: close\r\n\r\n",
"upper": b"GET / HTTP/1.1\r\nHost: example.org\r\nConnection: Close\r\n\r\n",
"mixed": b"GET / HTTP/1.1\r\nHost: example.org\r\nConnection: keep-alive, close\r\n\r\n",
}
async def main():
for name, cls in [
("h11", H11Protocol),
("httptools", HttpToolsProtocol),
("zttp", ZttpProtocol),
]:
for key, req in REQUESTS.items():
protocol = get_connected_protocol(
Response("Hello, world", media_type="text/plain"),
cls,
access_log=False,
)
protocol.data_received(req)
await protocol.loop.run_one()
buf = protocol.transport.buffer.lower()
print(
name,
key,
"closing=",
protocol.transport.is_closing(),
"echo=",
b"connection: close" in buf,
)
asyncio.run(main())ActualExpectedAll three implementations close after the response and echo CauseCLOSE_HEADER = (b"connection", b"close")
if CLOSE_HEADER in self.scope["headers"] and CLOSE_HEADER not in headers:
headers = headers + [CLOSE_HEADER]Header names are lowercased; values are not. QuestionDoes this match what you’d want fixed? If so, I can open a PR that parses |
Replies: 3 comments
|
Two things worth adding, because the fix is a bit wider than the httptools request check and the obvious one-liner doesn't cover all three of your cases. First, the exact-match constant isn't local to httptools. It's defined in CLOSE_HEADER = (b"connection", b"close")if CLOSE_HEADER in self.scope["headers"] and CLOSE_HEADER not in headers:
headers = headers + [CLOSE_HEADER]That's Second, lowercasing alone won't fix if name == b"connection":
connection = [token.lower().strip() for token in value.split(b",")]if b"upgrade" in connection:Split, strip, lowercase each token, membership test. That's exactly what the close check needs, and it makes the change an argument about consistency with existing code rather than a new convention — which is usually an easier sell. So the tidy shape is probably a small helper next to Worth extending the test matrix past your three too — |
|
Your diagnosis holds, and the shared-constant point above is right. Two more things fall out of the same line, and both are reachable today without any unusual request — worth folding into the PR so this gets fixed once. The response side isn't actually case-insensitive, it's case-insensitive for one exact value. Your write-up says httptools already handles the response elif name == b"connection" and value.lower() == b"close":
self.keep_alive = False
It also explains why the two halves aren't independent. The only thing that flips The dedupe guard on that same line has an exact-match bug of its own: if CLOSE_HEADER in self.scope["headers"] and CLOSE_HEADER not in headers:
headers = headers + [CLOSE_HEADER]
So there are three comparisons against the same concept, each wrong in a different way: exact-match on the request, exact-match on the app's headers, single-token equality on the response. A helper next to def has_close_token(headers: list[tuple[bytes, bytes]]) -> bool:
for name, value in headers:
if name.lower() == b"connection":
if b"close" in (token.strip() for token in value.lower().split(b",")):
return True
return FalseRequest side and app-header dedupe both become Two cases for the matrix beyond the ones already suggested. An app response of Small thing on your reproducer table: |
|
I merged a fix. |
I merged a fix.