fix(http): enable httptools lenient data

[vvanglro]

Summary

#2486

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.

[vvanglro]

@Kludex The process is hanging again. #2431

[Kludex]

Can you explain a bit what we are doing here?

[Kludex]

@Kludex The process is hanging again. #2431

I'll merge that after we fix this. 👍

[vvanglro]

Can you explain a bit what we are doing here?

Use a relaxed approach to parsing messages.

[vvanglro]

If we follow the discussion in #2238 and the RFC standard we should use lenient mode to parse the data.
Until then, we have been using the lenient mode to parse the data.

From RFC 9112, section 9.6:

A server that receives a "close" connection option MUST initiate closure of the connection (see below) after it sends the final response to the request that contained the "close" connection option. The server SHOULD send a "close" connection option in its final response on that connection. The server MUST NOT process any further requests received on that connection.

[vvanglro]

As a record.

Httptools v0.6.3 upgrades dependency llhttp from 8.1.1 to 9.2.1, due to the lenient mode used before llhttp v9.0, some messages could not be parsed and responded to in this upgrade.

The following is a list of methods that support lenient parsing in llhttp, facilitate subsequent troubleshooting and development of more lenient methods as needed.
https://github.com/nodejs/llhttp/tree/v9.2.1?tab=readme-ov-file#void-llhttp_set_lenient_headersllhttp_t-parser-int-enabled

[Kludex]

Should we enable this flag tho? https://llhttp.org/

Maybe we should fix the test instead?

[vvanglro]

Should we enable this flag tho? https://llhttp.org/

message = 'GET / HTTP/1.1\r\nConnection: close\r\n\r\nInvalid\r\n\r\n'

For enabling or not, I think it depends on whether the production environment receives such messages. If such messages are not generated except for malicious attacks, then we do not need to enable them.

Maybe we should fix the test instead?

Since h11 is now capable of handling such messages, if you want to modify it, keep the processing logic of h11 and httptools the same.

[vvanglro]

How about leaving it up to the user to decide whether or not to use a lenient flag. 🤔

[Kludex]

No. I don't think we should add a new flag.

[vvanglro]

Do you have any good suggestions?

import httptools

REQUEST_AFTER_CONNECTION_CLOSE = b"\r\n".join(
    [
        b"GET / HTTP/1.1",
        b"Host: example1.org",
        b"Connection: close",
        b"",
        b"",
        b"GET / HTTP/1.1",
        b"Host: example2.org",
        b"",
        b"",
    ]
)
class MyHandler:
    def on_url(self, url: bytes) -> None:
        print(f"Target: {url}")
    def on_headers_complete(self) -> None:
        http_version = parser.get_http_version()
        method = parser.get_method()
        print(f"HTTP version: {http_version}, method: {method}")
    def on_header(self, name: bytes, value: bytes) -> None:
        print(f"Header: {name.decode()}: {value.decode()}")

parser = httptools.HttpRequestParser(MyHandler())
# Adding this line works fine.
parser.set_dangerous_leniencies(lenient_data_after_close=True)
parser.feed_data(REQUEST_AFTER_CONNECTION_CLOSE)


import h11

conn = h11.Connection(h11.SERVER)
conn.receive_data(REQUEST_AFTER_CONNECTION_CLOSE)
print(conn.next_event())

Or we can turn this flag on by default, keeping h11 and httptools the same processing logic.

[Kludex]

Sorry, I shouldn't have force-pushed. I should have just added the revert commits. I used your first commit here.

[Kludex]

Some people don't use uvicorn[standard] because it install more than needed on production e.g. watchfiles.

[Kludex]

Thanks.