Commit 13b7dab
bneradt
Fix H3 quiche traffic handling
# Overview
This patch extends the HTTP/3 autest coverage, using curl, golang, and
Proxy Verifier HTTP/3 clients to generate their implementations of h3
traffic. It also adds request and response bodies of various sizes,
including "large" 300k bodies to exercise multiple packet, buffer, and
flow control ATS HTTP/3 implementations. It also exercises some
interesting requests, such as HEAD and 204 responses. This patch also
includes the various production fixes needed for these tests.
# Issues Found and their Fixes
## UDP batches could stall large H3 transfers
Large request and response bodies exposed a UDP receive starvation bug
in the UDP read path. On systems using `recvmmsg()` with
edge-triggered readiness, ATS could read one full batch of datagrams
and then leave the rest queued in the kernel without another readable
event to wake the QUIC stack.
This changes
`UDPNetProcessorInternal::read_multiple_messages_from_net()` in
`src/iocore/net/UnixUDPNet.cc` to return whether the kernel supplied a
full batch. `udp_read_from_net()` now loops while full batches are
returned, draining the socket before handing packets to QUIC and
avoiding large-transfer stalls caused by unread UDP bursts.
## QUIC stream writes consumed data before quiche accepted it
The stream write path consumed the `QUICStreamVCAdapter` write reader
inside `_read()`, before `QUICStream::send_data()` knew whether
`quiche_conn_stream_send()` had accepted the bytes. When quiche accepted
only a partial write or returned a flow-control error, ATS could lose
stream data and report write progress too early.
This makes `QUICStream::send_data()` keep a pending
`IOBufferBlock`/FIN pair until quiche reports successful consumption,
and only then calls the new `QUICStreamAdapter::consume()` hook. The
concrete reader accounting lives in `QUICStreamVCAdapter::_consume()`,
while `QUICStream::has_data_to_send()`, `QUICStream::on_write()`, and
`QUICNetVConnection::on_stream_updated()` make newly writable stream
data schedule packet writes again. This also treats completed finite
writes with only FIN left as writable stream state, so empty bodies and
fully consumed bodies still close the H3 stream cleanly.
## H3 transaction cleanup raced with stream closure
The timeout and stream lifetime tests exposed cases where an
`HQTransaction` could be deleted while an event handler was still
active, or while the QUIC stream adapter still had read/write cleanup to
finish. That left later stream-close and timeout paths touching state
that had already been torn down.
This adds explicit transaction lifetime state in `HQTransaction`:
`_closed`, `_stream_closed`, `_event_handler_active`, and
`_is_write_buffer_flushed()`. `Http3App::on_stream_close()` now calls
`HQTransaction::stream_closed()`, and
`HQTransaction::_delete_if_possible()` waits until the transaction is
done, the stream is closed or no longer readable, and pending writes
have flushed before deleting the transaction.
## H3 read completion could run before headers and DATA were settled
The H3 request read path could signal completion before asynchronous
QPACK header decode and buffered DATA delivery had finished updating the
sink VIO. That showed up around HEAD, 204, and stream-close timing
because the HTTP state machine needed a stable view of whether headers
were decoded and whether a request body existed.
This updates `Http3HeaderVIOAdaptor::_on_qpack_decode_complete()` to add
the printed header length to the sink VIO and notify
`Http3Transaction::on_header_decode_complete()`, which schedules the
appropriate read event. `Http3StreamDataVIOAdaptor::finalize()` now uses
a persistent reader, writes buffered DATA into the sink VIO exactly
once, and updates `ndone`/`nbytes` consistently before the transaction
is signaled.
## The QPACK static table had drifted from the standard table
The HEAD, 204, and quic-go coverage exposed that ATS's static QPACK
table was not the table used by external HTTP/3 implementations. The
extra zstd entry and modified `accept-encoding` value in
`src/proxy/http3/QPACK.cc` shifted later static indexes, so an
externally encoded `:status 204` could decode as a different status.
This restores the standard static table entries by using
`accept-encoding: gzip, deflate, br` and removing the non-standard
`content-encoding: zstd` entry. The new 204 cases in
`tests/gold_tests/h3/replays/h3_proxy_verifier.replay.yaml` and
`tests/gold_tests/h3/replays/h3_server_for_go_client.replay.yaml` cover
this interoperability point with Proxy Verifier and quic-go.1 parent fb1f03d commit 13b7dab
46 files changed
Lines changed: 2085 additions & 103 deletions
File tree
- ci
- include
- iocore/net/quic
- proxy
- http3
- logging
- src
- iocore/net
- quic
- proxy
- http3
- test
- logging
- tests/gold_tests
- autest-site
- h3
- go_h3_client
- replays
- timeout
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
| 27 | + | |
| 28 | + | |
27 | 29 | | |
28 | 30 | | |
29 | 31 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
| 31 | + | |
| 32 | + | |
31 | 33 | | |
32 | 34 | | |
33 | 35 | | |
| |||
191 | 193 | | |
192 | 194 | | |
193 | 195 | | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
194 | 201 | | |
195 | 202 | | |
196 | 203 | | |
| |||
431 | 438 | | |
432 | 439 | | |
433 | 440 | | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
434 | 446 | | |
435 | 447 | | |
436 | 448 | | |
| |||
519 | 531 | | |
520 | 532 | | |
521 | 533 | | |
522 | | - | |
523 | | - | |
| 534 | + | |
| 535 | + | |
524 | 536 | | |
525 | 537 | | |
526 | 538 | | |
527 | 539 | | |
528 | 540 | | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
529 | 547 | | |
530 | 548 | | |
531 | 549 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
60 | 60 | | |
61 | 61 | | |
62 | 62 | | |
| 63 | + | |
63 | 64 | | |
64 | 65 | | |
65 | 66 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| 29 | + | |
29 | 30 | | |
30 | 31 | | |
31 | 32 | | |
| |||
53 | 54 | | |
54 | 55 | | |
55 | 56 | | |
| 57 | + | |
56 | 58 | | |
57 | 59 | | |
58 | 60 | | |
| |||
66 | 68 | | |
67 | 69 | | |
68 | 70 | | |
| 71 | + | |
69 | 72 | | |
70 | 73 | | |
71 | 74 | | |
| |||
85 | 88 | | |
86 | 89 | | |
87 | 90 | | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
88 | 94 | | |
89 | 95 | | |
90 | 96 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
| 42 | + | |
42 | 43 | | |
43 | 44 | | |
44 | 45 | | |
| |||
60 | 61 | | |
61 | 62 | | |
62 | 63 | | |
63 | | - | |
| 64 | + | |
| 65 | + | |
64 | 66 | | |
65 | 67 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
65 | 65 | | |
66 | 66 | | |
67 | 67 | | |
| 68 | + | |
68 | 69 | | |
69 | 70 | | |
70 | 71 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
455 | 455 | | |
456 | 456 | | |
457 | 457 | | |
458 | | - | |
459 | | - | |
460 | | - | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
461 | 461 | | |
462 | 462 | | |
463 | 463 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
45 | | - | |
46 | | - | |
47 | | - | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
48 | 50 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| 56 | + | |
56 | 57 | | |
57 | 58 | | |
58 | 59 | | |
| |||
81 | 82 | | |
82 | 83 | | |
83 | 84 | | |
| 85 | + | |
84 | 86 | | |
85 | 87 | | |
86 | 88 | | |
| |||
90 | 92 | | |
91 | 93 | | |
92 | 94 | | |
| 95 | + | |
93 | 96 | | |
94 | 97 | | |
95 | 98 | | |
96 | 99 | | |
97 | 100 | | |
98 | 101 | | |
| 102 | + | |
99 | 103 | | |
100 | 104 | | |
101 | 105 | | |
| |||
106 | 110 | | |
107 | 111 | | |
108 | 112 | | |
109 | | - | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
110 | 117 | | |
111 | 118 | | |
112 | 119 | | |
| |||
121 | 128 | | |
122 | 129 | | |
123 | 130 | | |
| 131 | + | |
124 | 132 | | |
125 | 133 | | |
126 | 134 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
| 30 | + | |
| 31 | + | |
30 | 32 | | |
31 | 33 | | |
32 | 34 | | |
| |||
99 | 101 | | |
100 | 102 | | |
101 | 103 | | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
109 | 114 | | |
110 | 115 | | |
111 | 116 | | |
| |||
0 commit comments