You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/05/29 04:04:52 UTC
[PR] Fix H3 quiche large body handling [trafficserver]
bneradt opened a new pull request, #13213:
URL: https://github.com/apache/trafficserver/pull/13213
HTTP/3 over the quiche/BoringSSL path could stall or tear down under
larger request and response bodies. Proxy Verifier now has coverage for
body sizes that exceed initial flow-control windows, which exposed ATS
leaving UDP bursts unread and consuming stream write data before quiche
accepted it.
This keeps QUIC stream write buffers pending until quiche reports bytes
accepted, schedules packet writes when stream state changes, and keeps
H3 transactions alive until request and response cleanup is safe. This
also drains recvmmsg batches on edge-triggered UDP sockets so large
bursts do not wait for an event that may never arrive.
This adds replay coverage for H3 curl and Proxy Verifier scenarios,
including multiple connections, large GET and POST bodies, and timeout
and lifetime cleanup cases. The large-body Proxy Verifier test depends
on yahoo/proxy-verifier#385.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3531281772
##########
tests/gold_tests/h3/py_h3_client/h3_client.py:
##########
@@ -0,0 +1,332 @@
+#!/usr/bin/env python3
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Exercise ATS HTTP/3 client-side behavior with aioquic."""
+
+from __future__ import annotations
+
+import argparse
+import asyncio
+import ssl
+from dataclasses import dataclass, field
+from typing import Awaitable, Callable
Review Comment:
Fixed.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3365962396
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -217,6 +231,29 @@ def HasProxyVerifierVersion(self, version):
return self.EnsureVersion([verifier_path, "--version"], min_version=version)
+def HasGoVersion(self, version):
+ """Check whether the go command is available at the requested version."""
+
+ def version_tuple(value):
+ return tuple(int(part) for part in value.split('.'))
+
+ def check_go_version():
+ try:
+ output = subprocess.check_output(["go", "version"], stderr=subprocess.STDOUT, text=True)
+ except (OSError, subprocess.SubprocessError):
+ return False
+
+ match = re.search(r'\bgo(\d+\.\d+(?:\.\d+)?)\b', output)
+ if match is None:
+ return False
+
+ found = version_tuple(match.group(1))
+ required = version_tuple(version)
+ return found >= required
+
+ return self.Condition(check_go_version, "Go must be installed and at least version " + version)
Review Comment:
Fixed. HasGoVersion() now uses the same padded _version_tuple() helper, so go1.24 satisfies a 1.24.0 requirement instead of failing due to unequal tuple length comparison.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-5131509534
[approve ci fedora]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3530927433
##########
tests/gold_tests/h3/py_h3_client/h3_client.py:
##########
@@ -0,0 +1,332 @@
+#!/usr/bin/env python3
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Exercise ATS HTTP/3 client-side behavior with aioquic."""
+
+from __future__ import annotations
+
+import argparse
+import asyncio
+import ssl
+from dataclasses import dataclass, field
+from typing import Awaitable, Callable
Review Comment:
`Awaitable` is imported but never used in this module. This can trigger pyflakes/lint failures in the test harness and adds noise to the dependency surface.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3366292752
##########
src/proxy/http3/Http3App.cc:
##########
@@ -131,6 +131,10 @@ Http3App::on_stream_open(QUICStream &stream)
void
Http3App::on_stream_close(QUICStream &stream)
{
+ if (auto *txn = this->_ssn->get_transaction(stream.id()); txn != nullptr) {
+ SCOPED_MUTEX_LOCK(lock, txn->mutex, this_ethread());
+ txn->stream_closed();
+ }
this->_streams.erase(stream.id());
Review Comment:
`Http3Transaction` stores a reference to `QUICStreamVCAdapter::IOInfo` (from `Http3App::_streams`). In `on_stream_close()` the IOInfo is erased unconditionally after calling `txn->stream_closed()`. If the transaction cannot delete immediately (e.g. `_event_handler_active` is true, or `_transaction_done` is still false), the transaction can continue to access `_info` after the map erase, causing a use-after-free of the IOInfo and its VIO/buffers.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4636925294
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4663521363
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3327312931
##########
src/iocore/net/quic/QUICStream.cc:
##########
@@ -122,15 +136,41 @@ QUICStream::send_data(quiche_conn *quiche_con)
if (len <= 0) {
return;
}
- Ptr<IOBufferBlock> block = this->_adapter->read(len);
- if (this->_adapter->total_len() == this->_sent_bytes + block->size()) {
- fin = true;
+
+ if (!this->_pending_send_block) {
+ this->_pending_send_block = this->_adapter->read(len);
+ if (!this->_pending_send_block) {
+ this->_adapter->encourge_write();
+ return;
Review Comment:
Fixed. QUICStream::has_data_to_send() now treats EOS with all bytes sent but no FIN as pending output, and QUICStream::send_data() sends a zero-length STREAM frame with FIN for that case while tracking _sent_fin.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4596038325
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3360366206
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -136,6 +136,20 @@ def HasOpenSSLVersion(self, version):
return self.Condition(lambda: exe_ver >= version, "OpenSSL library version is " + exe_ver + ", must be at least " + version)
Review Comment:
`HasOpenSSLVersion()` compares versions as plain strings (`exe_ver >= version`), which is lexicographic and can mis-order multi-digit components (e.g. "3.10.0" is considered < "3.5.0"). This can incorrectly skip or run tests (including the new HTTP/3 session ticket coverage) as OpenSSL 3.x minor versions increase. Parse the versions into integer tuples before comparing.
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -217,6 +231,29 @@ def HasProxyVerifierVersion(self, version):
return self.EnsureVersion([verifier_path, "--version"], min_version=version)
+def HasGoVersion(self, version):
+ """Check whether the go command is available at the requested version."""
+
+ def version_tuple(value):
+ return tuple(int(part) for part in value.split('.'))
+
+ def check_go_version():
+ try:
+ output = subprocess.check_output(["go", "version"], stderr=subprocess.STDOUT, text=True)
+ except (OSError, subprocess.SubprocessError):
+ return False
+
+ match = re.search(r'\bgo(\d+\.\d+(?:\.\d+)?)\b', output)
+ if match is None:
+ return False
+
+ found = version_tuple(match.group(1))
+ required = version_tuple(version)
+ return found >= required
+
+ return self.Condition(check_go_version, "Go must be installed and at least version " + version)
Review Comment:
`HasGoVersion()` compares tuples of unequal lengths directly. In Python, `(1, 24) < (1, 24, 0)`, so passing a patch-qualified minimum like `"1.24.0"` would incorrectly fail even when `go version` reports `go1.24`. Padding both sides to the same width (e.g. 3 components) avoids false negatives.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4782492152
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3365965400
##########
src/proxy/http3/Http3Session.cc:
##########
@@ -162,9 +162,16 @@ HQSession::main_event_handler(int event, void *edata)
case VC_EVENT_ERROR:
case VC_EVENT_EOS:
this->do_io_close();
- for (HQTransaction *t = this->_transaction_list.head; t; t = static_cast<HQTransaction *>(t->link.next)) {
+ while (true) {
+ HQTransaction *t = this->_transaction_list.head;
+ if (t == nullptr) {
+ break;
+ }
SCOPED_MUTEX_LOCK(lock, t->mutex, this_ethread());
t->handleEvent(event, edata);
+ if (this->_transaction_list.head == t) {
+ break;
+ }
Review Comment:
HQSession::main_event_handler() now walks _transaction_list with a saved next pointer before calling handleEvent(), so later HTTP/3 transactions still receive the close/error event even if the current transaction survives.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4627576473
[approve ci osx]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3327326695
##########
src/iocore/net/quic/QUICStreamVCAdapter.cc:
##########
@@ -321,23 +345,34 @@ QUICStreamVCAdapter::do_io_shutdown(ShutdownHowTo_t /* howto ATS_UNUSED */)
}
void
-QUICStreamVCAdapter::reenable(VIO * /* vio ATS_UNUSED */)
+QUICStreamVCAdapter::reenable(VIO *vio)
{
- // TODO We probably need to tell QUICStream that the application consumed received data
- // to update receive window here. In other words, we should not update receive window
- // until the application consume data.
+ if (vio == nullptr || vio->op != VIO::WRITE) {
+ // TODO We probably need to tell QUICStream that the application consumed received data
+ // to update receive window here. In other words, we should not update receive window
+ // until the application consume data.
+ return;
+ }
+
+ const bool has_buffered_data = vio->get_reader() != nullptr && vio->get_reader()->read_avail() > 0;
+ const bool needs_fin = vio->nbytes != INT64_MAX && vio->ntodo() == 0;
+ if (has_buffered_data || needs_fin) {
+ this->stream().on_write();
+ }
}
bool
QUICStreamVCAdapter::is_readable()
{
- return this->stream().direction() != QUICStreamDirection::SEND && _read_vio.nbytes != _read_vio.ndone;
+ return this->stream().direction() != QUICStreamDirection::SEND && this->_read_vio.op == VIO::READ &&
+ this->_read_vio.nbytes != this->_read_vio.ndone;
}
bool
QUICStreamVCAdapter::is_writable()
{
- return this->stream().direction() != QUICStreamDirection::RECEIVE && _write_vio.nbytes != _read_vio.ndone;
+ return this->stream().direction() != QUICStreamDirection::RECEIVE && this->_write_vio.op == VIO::WRITE &&
+ this->_write_vio.nbytes != this->_write_vio.ndone;
Review Comment:
Fixed. QUICStreamVCAdapter::is_writable() now stays tied to QUIC stream direction, so a normal bidirectional request stream is not deleted before the response write VIO is installed. Pending write/FIN scheduling remains handled separately by reenable() and the stream send path.
##########
tests/gold_tests/h3/h3_session_ticket.test.py:
##########
@@ -0,0 +1,108 @@
+'''
+Verify HTTP/3 QUIC TLS session ticket handling.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import shlex
+
+Test.Summary = '''
+Verify that HTTP/3 QUIC connections can receive and offer TLS session tickets.
+'''
+
+Test.SkipUnless(
+ Condition.HasATSFeature('TS_USE_QUIC'),
+ Condition.HasOpenSSLVersion('3.5.0'),
Review Comment:
Fixed. I added Condition.HasOpenSSLQuicClient(), which checks that the openssl command on PATH supports s_client -quic, and added it to h3_session_ticket alongside the ATS-linked OpenSSL 3.5 gate.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche large body handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3322081401
##########
tests/gold_tests/h3/h3_stream_lifetime.test.py:
##########
@@ -0,0 +1,27 @@
+'''
+Verify HTTP/3 stream lifetime handling with concurrent streams.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Verify HTTP/3 transactions survive concurrent stream close and write-ready
+events on the same QUIC connection.
+'''
+
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_QUIC'))
Review Comment:
This HTTP/3 replay also depends on Proxy Verifier support for HTTP/3 sessions, but the skip condition only checks the ATS build. Please gate this on the Proxy Verifier version/feature that added HTTP/3 replay support so older verifier installations skip the test instead of failing during setup or client execution.
##########
tests/gold_tests/h3/h3_proxy_verifier.test.py:
##########
@@ -0,0 +1,27 @@
+'''
+Verify HTTP/3 client interop with Proxy Verifier.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Verify a real HTTP/3 Proxy Verifier client can complete multiple transactions
+across multiple QUIC connections through ATS.
+'''
+
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_QUIC'))
Review Comment:
This replay uses HTTP/3 Proxy Verifier behavior that the PR description notes depends on yahoo/proxy-verifier#385, but the test only gates on ATS QUIC support. On systems with an older `verifier-client`, the test will run and fail before it exercises ATS; please add a `Condition.HasProxyVerifierVersion(...)` (or equivalent feature gate) for the Proxy Verifier release that contains the HTTP/3 replay support.
##########
tests/gold_tests/h3/h3_active_timeout.test.py:
##########
@@ -0,0 +1,26 @@
+'''
+Verify HTTP/3 transaction cleanup on active timeout.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Verify HTTP/3 transactions are removed cleanly after transaction active timeout.
+'''
+
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_QUIC'))
Review Comment:
This HTTP/3 replay depends on Proxy Verifier support for HTTP/3 sessions, but the skip condition only checks the ATS build. Please add a Proxy Verifier version/feature gate here as well so environments with older verifier binaries skip the test instead of failing before validating the timeout behavior.
##########
src/iocore/net/quic/QUICStreamVCAdapter.cc:
##########
@@ -321,8 +345,12 @@ QUICStreamVCAdapter::do_io_shutdown(ShutdownHowTo_t /* howto ATS_UNUSED */)
}
void
-QUICStreamVCAdapter::reenable(VIO * /* vio ATS_UNUSED */)
+QUICStreamVCAdapter::reenable(VIO *vio)
{
+ if (vio != nullptr && vio->op == VIO::WRITE && vio->get_reader() != nullptr && vio->get_reader()->read_avail() > 0) {
+ this->stream().on_write();
+ }
Review Comment:
A completed write VIO can require sending only the QUIC FIN after all buffered frame bytes were already accepted (for example, an empty response body after HEADERS were flushed). This condition only schedules packet output when `read_avail() > 0`, so that zero-byte completion does not wake the QUIC connection to emit FIN. Include the `vio->ntodo() == 0`/finite-complete case in the update condition.
##########
src/iocore/net/quic/QUICStream.cc:
##########
@@ -122,15 +136,41 @@ QUICStream::send_data(quiche_conn *quiche_con)
if (len <= 0) {
return;
}
- Ptr<IOBufferBlock> block = this->_adapter->read(len);
- if (this->_adapter->total_len() == this->_sent_bytes + block->size()) {
- fin = true;
+
+ if (!this->_pending_send_block) {
+ this->_pending_send_block = this->_adapter->read(len);
+ if (!this->_pending_send_block) {
+ this->_adapter->encourge_write();
+ return;
Review Comment:
When the HTTP/3 frame collector later marks the lower write VIO complete without adding more bytes (for example after response HEADERS have already been flushed and the response has an empty body), `_adapter->read(len)` returns null even though `total_len() == _sent_bytes`. This path only encourages another write and never sends a zero-length STREAM frame with FIN, so the peer can wait indefinitely for end-of-stream on zero-body responses whose headers were sent before `all_done` became true. Handle the no-block-but-all-bytes-sent case by sending FIN instead of returning here.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3368723864
##########
src/proxy/http3/Http3Transaction.cc:
##########
@@ -612,7 +731,7 @@ Http3Transaction::_process_write_vio()
auto error = this->_frame_collector.on_write_ready(this->_info.adapter.stream().id(), *this->_info.write_vio->get_writer(),
nwritten, all_done);
if (error && error->cls != Http3ErrorClass::UNDEFINED) {
- Http3TransDebug("Error occured while processing write vio: %hu, %s", error->get_code(), error->msg);
+ Http3TransDebug("Error occured while processing write vio: %hu", error->get_code());
return 0;
Review Comment:
Typo in the debug message: "occured" should be "occurred" for consistency with the read-path log and to improve searchability.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-5144754272
Cherry-picked to the 10.2.x branch as 5e248c31221dcbf5468e2dca60f225e247c797bc for the 10.2.0 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche large body handling [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4578826885
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4895449844
> I don't want any changes for H2 on this PR. Please make those changes on a separate PR.
Yeah, separating is a good idea. Here's the separate PR:
https://github.com/apache/trafficserver/pull/13363
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3532274751
##########
include/proxy/http3/Http3Transaction.h:
##########
@@ -106,7 +114,12 @@ class HQTransaction : public ProxyTransaction
Event *_write_ready_event = nullptr;
Event *_write_complete_event = nullptr;
- bool _transaction_done = false;
+ bool _transaction_done = false;
+ bool _event_handler_active = false;
+ bool _closed = false;
+ bool _stream_closed = false;
Review Comment:
Addressed. I removed the _closed and _stream_closed booleans from HQTransaction. The transaction closed state is now derived from the transaction continuation handler, and the QUIC stream closed state is derived from the QUICStreamVCAdapter handler. I verified this against both BoringSSL+quiche and OpenSSL native QUIC H3 AuTests.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-5131489182
[approve ci fedora]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3422521581
##########
include/proxy/http3/Http3Transaction.h:
##########
@@ -106,7 +114,12 @@ class HQTransaction : public ProxyTransaction
Event *_write_ready_event = nullptr;
Event *_write_complete_event = nullptr;
- bool _transaction_done = false;
+ bool _transaction_done = false;
+ bool _event_handler_active = false;
+ bool _closed = false;
+ bool _stream_closed = false;
Review Comment:
I don't like these closed flags. I know H2 has similar ones, but those are just confusing. Places that needs these flags should consider checking QUIC stream state.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4637197235
[approve ci freebsd]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3366370739
##########
src/proxy/http3/Http3App.cc:
##########
@@ -131,6 +131,10 @@ Http3App::on_stream_open(QUICStream &stream)
void
Http3App::on_stream_close(QUICStream &stream)
{
+ if (auto *txn = this->_ssn->get_transaction(stream.id()); txn != nullptr) {
+ SCOPED_MUTEX_LOCK(lock, txn->mutex, this_ethread());
+ txn->stream_closed();
+ }
this->_streams.erase(stream.id());
Review Comment:
Http3App::on_stream_close() now installs a cleanup callback on the transaction while holding the transaction mutex. _streams is only erased immediately when no transaction exists; otherwise the IOInfo remains alive until HQTransaction destruction runs the cleanup.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-5137295099
The following PR contains the 10.2.x version of this:
https://github.com/apache/trafficserver/pull/13463
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-4626740720
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche large body handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3326766242
##########
tests/gold_tests/h3/replays/h3_active_timeout.replay.yaml:
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: "Verify HTTP/3 transaction cleanup on active timeout"
+
+ server:
+ name: "server-h3-active-timeout"
+
+ client:
+ name: "client-h3-active-timeout"
+ return_code: 1
+
+ ats:
+ name: "ts-h3-active-timeout"
+ process_config:
+ enable_tls: true
+ enable_quic: true
+
+ records_config:
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: "quic|http"
+ proxy.config.http.transaction_active_timeout_in: 1
+ proxy.config.quic.no_activity_timeout_in: 0
+ proxy.config.quic.server.stateless_retry_enabled: 0
+
+ remap_config:
+ - from: "https://example.com/"
+ to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+
+sessions:
+- protocol:
+ - name: http
+ version: 3
+ - name: tls
+ sni: example.com
+ - name: udp
+ - name: ip
+
+ transactions:
+ - client-request:
+ version: "3"
+ headers:
+ fields:
+ - [ ":method", GET ]
+ - [ ":scheme", https ]
+ - [ ":authority", example.com ]
+ - [ ":path", /h3-active-timeout ]
+ - [ uuid, h3-active-timeout ]
+
+ server-response:
+ delay: 2s
Review Comment:
Add a one sentence comment explaining this exercises the 1 second timeout.
##########
tests/gold_tests/h3/h3_active_timeout.test.py:
##########
@@ -0,0 +1,26 @@
+'''
+Verify HTTP/3 transaction cleanup on active timeout.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Verify HTTP/3 transactions are removed cleanly after transaction active timeout.
+'''
+
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_QUIC'))
Review Comment:
Nah. All Proxy Verifier binary releases support HTTP/3 QUIC for the last five years or so.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3365963205
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -136,6 +136,20 @@ def HasOpenSSLVersion(self, version):
return self.Condition(lambda: exe_ver >= version, "OpenSSL library version is " + exe_ver + ", must be at least " + version)
Review Comment:
Fixed. I added a shared _version_tuple() helper and changed HasOpenSSLVersion() to extract the dotted OpenSSL version and compare padded integer tuples, so versions like 3.10.0 sort correctly against 3.5.0.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3365964972
##########
src/iocore/net/quic/QUICStreamAdapter.cc:
##########
@@ -26,7 +26,12 @@
Ptr<IOBufferBlock>
QUICStreamAdapter::read(size_t len)
{
- auto ret = this->_read(len);
+ return this->_read(len);
+}
+
+void
+QUICStreamAdapter::consume(size_t len)
+{
+ this->_consume(len);
Review Comment:
QUICStreamAdapter::read() is now a peek-style read, and direct draining callers consume explicitly: TestQUICStream::read() in test_QPACK.cc calls consume(nread), while QUICStream::send_data() consumes only the bytes accepted by quiche.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3327303867
##########
src/iocore/net/quic/QUICStreamVCAdapter.cc:
##########
@@ -321,23 +345,34 @@ QUICStreamVCAdapter::do_io_shutdown(ShutdownHowTo_t /* howto ATS_UNUSED */)
}
void
-QUICStreamVCAdapter::reenable(VIO * /* vio ATS_UNUSED */)
+QUICStreamVCAdapter::reenable(VIO *vio)
{
- // TODO We probably need to tell QUICStream that the application consumed received data
- // to update receive window here. In other words, we should not update receive window
- // until the application consume data.
+ if (vio == nullptr || vio->op != VIO::WRITE) {
+ // TODO We probably need to tell QUICStream that the application consumed received data
+ // to update receive window here. In other words, we should not update receive window
+ // until the application consume data.
+ return;
+ }
+
+ const bool has_buffered_data = vio->get_reader() != nullptr && vio->get_reader()->read_avail() > 0;
+ const bool needs_fin = vio->nbytes != INT64_MAX && vio->ntodo() == 0;
+ if (has_buffered_data || needs_fin) {
+ this->stream().on_write();
+ }
}
bool
QUICStreamVCAdapter::is_readable()
{
- return this->stream().direction() != QUICStreamDirection::SEND && _read_vio.nbytes != _read_vio.ndone;
+ return this->stream().direction() != QUICStreamDirection::SEND && this->_read_vio.op == VIO::READ &&
+ this->_read_vio.nbytes != this->_read_vio.ndone;
}
bool
QUICStreamVCAdapter::is_writable()
{
- return this->stream().direction() != QUICStreamDirection::RECEIVE && _write_vio.nbytes != _read_vio.ndone;
+ return this->stream().direction() != QUICStreamDirection::RECEIVE && this->_write_vio.op == VIO::WRITE &&
+ this->_write_vio.nbytes != this->_write_vio.ndone;
Review Comment:
`is_writable()` is used by `Http3App::_handle_bidi_stream_on_read_complete()` to decide whether a bidirectional request stream can be deleted. With the new `op == VIO::WRITE && nbytes != ndone` check, a normal server-side stream whose request has completed but whose response write has not been installed yet reports false, so `delete_stream()` closes the transaction before ATS can send the response. Keep this helper tied to the QUIC stream direction rather than the current VIO progress, or use a separate helper for pending write data.
##########
tests/gold_tests/h3/h3_session_ticket.test.py:
##########
@@ -0,0 +1,108 @@
+'''
+Verify HTTP/3 QUIC TLS session ticket handling.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import shlex
+
+Test.Summary = '''
+Verify that HTTP/3 QUIC connections can receive and offer TLS session tickets.
+'''
+
+Test.SkipUnless(
+ Condition.HasATSFeature('TS_USE_QUIC'),
+ Condition.HasOpenSSLVersion('3.5.0'),
Review Comment:
This gate checks the OpenSSL library linked into `traffic_server` via `traffic_layout`, but the test actually runs the `openssl` command from `PATH` and requires that CLI to support `s_client -quic`. If the build links against OpenSSL 3.5 while the executable on `PATH` is older or lacks QUIC, the test will run and fail before validating ATS. Please gate on the OpenSSL CLI capability/version used by `h3_session_ticket.sh` as well.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche large body handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3326824279
##########
tests/gold_tests/h3/h3_proxy_verifier.test.py:
##########
@@ -0,0 +1,27 @@
+'''
+Verify HTTP/3 client interop with Proxy Verifier.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information regarding
+# copyright ownership. The ASF licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may
+# obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Verify a real HTTP/3 Proxy Verifier client can complete multiple transactions
+across multiple QUIC connections through ATS.
+'''
+
+Test.SkipUnless(Condition.HasATSFeature('TS_USE_QUIC'))
Review Comment:
We don't typically do this for PV releases because we explicitly version the PV release we use in the source tree.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3365962396
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -217,6 +231,29 @@ def HasProxyVerifierVersion(self, version):
return self.EnsureVersion([verifier_path, "--version"], min_version=version)
+def HasGoVersion(self, version):
+ """Check whether the go command is available at the requested version."""
+
+ def version_tuple(value):
+ return tuple(int(part) for part in value.split('.'))
+
+ def check_go_version():
+ try:
+ output = subprocess.check_output(["go", "version"], stderr=subprocess.STDOUT, text=True)
+ except (OSError, subprocess.SubprocessError):
+ return False
+
+ match = re.search(r'\bgo(\d+\.\d+(?:\.\d+)?)\b', output)
+ if match is None:
+ return False
+
+ found = version_tuple(match.group(1))
+ required = version_tuple(version)
+ return found >= required
+
+ return self.Condition(check_go_version, "Go must be installed and at least version " + version)
Review Comment:
Fixed. I added a shared _version_tuple() helper and changed HasOpenSSLVersion() to extract the dotted OpenSSL version and compare padded integer tuples, so versions like 3.10.0 sort correctly against 3.5.0.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3327314225
##########
src/iocore/net/quic/QUICStreamVCAdapter.cc:
##########
@@ -321,8 +345,12 @@ QUICStreamVCAdapter::do_io_shutdown(ShutdownHowTo_t /* howto ATS_UNUSED */)
}
void
-QUICStreamVCAdapter::reenable(VIO * /* vio ATS_UNUSED */)
+QUICStreamVCAdapter::reenable(VIO *vio)
{
+ if (vio != nullptr && vio->op == VIO::WRITE && vio->get_reader() != nullptr && vio->get_reader()->read_avail() > 0) {
+ this->stream().on_write();
+ }
Review Comment:
Fixed. reenable() now wakes the QUIC stream when either buffered write data exists or a finite write VIO has completed with only FIN left to send.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix H3 quiche traffic handling [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#discussion_r3329047071
##########
src/iocore/net/quic/QUICStreamAdapter.cc:
##########
@@ -26,7 +26,12 @@
Ptr<IOBufferBlock>
QUICStreamAdapter::read(size_t len)
{
- auto ret = this->_read(len);
+ return this->_read(len);
+}
+
+void
+QUICStreamAdapter::consume(size_t len)
+{
+ this->_consume(len);
Review Comment:
Changing `read()` to only peek leaves existing direct callers that drain a `QUICStreamAdapter` without calling `consume()` stuck reading the same bytes repeatedly; for example `src/proxy/http3/test/test_QPACK.cc` loops on `adapter->read()` until it returns 0, but `MockQUICStreamAdapter::_read()` no longer decreases `_sending_data_len`. Update those callers/tests to consume the returned length (or keep the old consuming API and add a separate peek API for `send_data()`).
##########
src/proxy/http3/Http3Session.cc:
##########
@@ -162,9 +162,16 @@ HQSession::main_event_handler(int event, void *edata)
case VC_EVENT_ERROR:
case VC_EVENT_EOS:
this->do_io_close();
- for (HQTransaction *t = this->_transaction_list.head; t; t = static_cast<HQTransaction *>(t->link.next)) {
+ while (true) {
+ HQTransaction *t = this->_transaction_list.head;
+ if (t == nullptr) {
+ break;
+ }
SCOPED_MUTEX_LOCK(lock, t->mutex, this_ethread());
t->handleEvent(event, edata);
+ if (this->_transaction_list.head == t) {
+ break;
+ }
Review Comment:
This loop stops after notifying only the head transaction whenever that transaction survives the close/error event, so later HTTP/3 streams on the same session never receive the EOS/error cleanup event. Iterate over the list using a saved `next` pointer instead of breaking when the head is unchanged.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213#issuecomment-5125951824
[approve ci fedora]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add H3 quiche traffic handling tests and provide fixes [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13213:
URL: https://github.com/apache/trafficserver/pull/13213
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org