Spawn H2 Data frame processing into a separate task

[yaroslavros]

I noticed that under load DoH server once in a while goes into deadlock. This is particularly visible when ECH is enabled in Chrome and it makes three simultaneous requests in a single DoH session (for A, AAAA and HTTPS records). If all Data frames do not arrive at the same time as Header frames, DoH server goes into deadlock and never responds to requests with missing data frames.

Spawning Data frame processing into a separate Tokio task resolves this issue. Same logic is actually used in the basic h2 server example at https://github.com/hyperium/h2/blob/master/examples/server.rs (line 34).

[djc]

The changes seem reasonable but I'd like to better understand why this is necessary.

In HTTP land, I guess the more common strategy is to spawn a task per client (connection) rather than one per request -- at least, that's how hyper does things, which makes sense to me.

Have you looked at how this is handled for the other protocols? We should probably use the same strategy across protocols unless there are overriding reasons to do different things.

[yaroslavros]

This is not about efficiency but about preventing a deadlock.
Underlying H2 body stream that we extract in https://github.com/bluejekyll/trust-dns/blob/90edda6b0558282abf852494d7c4f4004e47e1d3/crates/proto/src/https/https_server.rs#L52 only streams from received buffer, https://github.com/bluejekyll/trust-dns/blob/90edda6b0558282abf852494d7c4f4004e47e1d3/crates/proto/src/https/https_server.rs#L68 does not progress underlying socket and if we don't have complete request in the buffer immediately (which happens in case of multiple concurrent requests) it just hangs.
The way to resolve this is to keep progressing the socket which is what this PR accomplishes.

Key difference between DoH and other protocols is that it can get multiple concurrent in-flight requests on a single socket.

[djc]

Okay, so if I understand correctly the model here is that we have to keep pulling frames off the H2 connection so that it is able to (in addition to new requests) read further body chunks for previous requests?

[djc]

I think this is the relevant bit of documentation from the h2 crate:

The Connection instance is used to manage connection state. The caller is required to call either Connection::accept or Connection::poll_close in order to advance the connection state. Simply operating on SendStream or RecvStream will have no effect unless the connection state is advanced.

[yaroslavros]

Exactly. By not calling Connection::accept we are blocking new data frames.

[djc]

(The CI failures are unrelated, and addressed in #2028.)

[bluejekyll]

This looks good. I'm rebasing, assuming tests are passing I'll merge after. Thanks for the PR!