Copilot commented on code in PR #14001:
URL: https://github.com/apache/trafficserver/pull/14001#discussion_r4476689496
##########
include/proxy/http3/QPACK.h:
##########
@@ -65,13 +65,25 @@ class QPACK : public QUICApplication
int encode(uint64_t stream_id, HTTPHdr &header_set, MIOBuffer *header_block,
uint64_t &header_block_len);
/*
- * This will emit either of two events below:
- * - QPACK_EVENT_DECODE_COMPLETE (Data: *HTTPHdr)
- * - QPACK_EVENT_DECODE_FAILED (Data: nullptr)
+ * This will emit either of two events below, on @a thread:
+ * - QPACK_EVENT_DECODE_COMPLETE (the event's cookie is &hdr)
+ * - QPACK_EVENT_DECODE_FAILED (the event's cookie is &hdr, or nullptr if the
+ * decoder had already failed)
+ *
+ * @a thread has to be the thread decode() is called on: QPACK keeps the
+ * results it has not delivered yet on a list that only that thread touches.
*/
int decode(uint64_t stream_id, const uint8_t *header_block, size_t
header_block_len, HTTPHdr &hdr, Continuation *cont,
EThread *thread = this_ethread());
Review Comment:
The updated contract states `thread` must be the same thread `decode()` is
called on, and `QPACK::decode()` now asserts `thread == this_ethread()`.
Keeping `thread` as a public parameter is now misleading and invites incorrect
usage (passing a different thread will assert at runtime). Consider removing
the parameter, making it internal-only, or at least updating the signature/API
to prevent callers from providing a mismatched thread (e.g., no parameter, or a
stronger type/alternative entry point).
##########
src/proxy/http3/Http3HeaderVIOAdaptor.cc:
##########
@@ -45,6 +45,11 @@ Http3HeaderVIOAdaptor::Http3HeaderVIOAdaptor(VIO *sink,
HTTPType http_type, QPAC
Http3HeaderVIOAdaptor::~Http3HeaderVIOAdaptor()
{
+ // QPACK may still hold this adaptor and its header, for a decode that is
+ // blocked or whose result is still in the event queue.
+ if (this->_qpack != nullptr) {
Review Comment:
With the new thread-affinity requirements (decode/cancel must run on the
delivery thread), calling `cancel()` directly from this destructor is only safe
if the adaptor is guaranteed to be destroyed on the same event thread that ran
`decode()`. If that guarantee exists, consider asserting it here for safety;
otherwise, marshal the cancel onto the correct event thread to avoid races with
QPACK’s pending report/block lists.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -310,6 +319,68 @@ QPACK::decode(uint64_t stream_id, const uint8_t
*header_block, size_t header_blo
return 0;
}
+int
+QPACK::cancel(uint64_t stream_id)
+{
+ int dropped = 0;
+
+ for (DecodeReport *r = this->_pending_reports.head(); r != nullptr;) {
+ DecodeReport *next = DecodeReport::Linkage::next_ptr(r);
+ if (r->stream_id() == stream_id) {
+ // The report is still in the event queue and frees itself when it runs.
+ this->_pending_reports.erase(r);
+ r->withdraw();
+ ++dropped;
+ }
+ r = next;
+ }
+
+ for (DecodeRequest *r = this->_blocked_list.head(); r != nullptr;) {
+ DecodeRequest *next = DecodeRequest::Linkage::next_ptr(r);
+ if (r->stream_id() == stream_id) {
+ this->_blocked_list.erase(r);
+ delete r;
+ ++dropped;
+ }
+ r = next;
+ }
+
+ return dropped;
+}
+
+QPACK::DecodeReport::DecodeReport(QPACK *qpack, Continuation *continuation,
uint64_t stream_id, int event, HTTPHdr *hdr)
+ : Continuation(continuation->mutex), _qpack(qpack),
_continuation(continuation), _stream_id(stream_id), _event(event), _hdr(hdr)
+{
+ SET_HANDLER(&QPACK::DecodeReport::deliver);
+}
+
+int
+QPACK::DecodeReport::deliver(int /* event ATS_UNUSED */, Event *data)
+{
+ // Off the list before the handler runs: the handler may destroy its owner,
+ // which cancels this stream, or the session that owns this QPACK.
+ if (this->_qpack != nullptr) {
+ this->_qpack->_pending_reports.erase(this);
+ }
+ if (this->_continuation != nullptr) {
+ data->cookie = this->_hdr;
+ this->_continuation->handleEvent(this->_event, data);
+ }
+ delete this;
+ return EVENT_DONE;
+}
Review Comment:
This forwards the `Event*` that was scheduled for `DecodeReport` into the
consumer continuation. That means fields on `data` (notably the event’s
associated continuation / cancellation semantics) no longer correspond to the
consumer continuation, which can break handlers that assume the `Event*` they
receive represents *their* scheduled event. A safer pattern is to schedule the
consumer continuation with the desired event/cookie (creating an `Event` owned
by the consumer) rather than calling `handleEvent()` directly with the
wrapper’s `Event*`.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -310,6 +319,68 @@ QPACK::decode(uint64_t stream_id, const uint8_t
*header_block, size_t header_blo
return 0;
}
+int
+QPACK::cancel(uint64_t stream_id)
+{
+ int dropped = 0;
+
+ for (DecodeReport *r = this->_pending_reports.head(); r != nullptr;) {
+ DecodeReport *next = DecodeReport::Linkage::next_ptr(r);
+ if (r->stream_id() == stream_id) {
+ // The report is still in the event queue and frees itself when it runs.
+ this->_pending_reports.erase(r);
+ r->withdraw();
+ ++dropped;
+ }
+ r = next;
+ }
+
+ for (DecodeRequest *r = this->_blocked_list.head(); r != nullptr;) {
+ DecodeRequest *next = DecodeRequest::Linkage::next_ptr(r);
+ if (r->stream_id() == stream_id) {
+ this->_blocked_list.erase(r);
+ delete r;
+ ++dropped;
+ }
+ r = next;
+ }
+
+ return dropped;
+}
Review Comment:
`cancel()` mutates `_pending_reports` and `_blocked_list`, which (per the
new docs) are intended to be touched only by the decode/delivery thread. Unlike
`decode()` / `_report()`, `cancel()` currently has no thread-affinity
assertion/guard, so an off-thread destructor call could race with delivery and
list iteration. Add the same kind of thread-affinity enforcement used by
`decode()`/`_report()` (e.g., record the owning thread in `QPACK` and
`ink_assert(this_ethread() == owning_thread)` in `cancel()`), or change
`cancel()` to marshal the work onto the owning thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
