Copilot commented on code in PR #13186:
URL: https://github.com/apache/trafficserver/pull/13186#discussion_r3329049838
##########
src/iocore/net/UnixUDPNet.cc:
##########
@@ -666,14 +666,16 @@
UDPNetProcessorInternal::read_multiple_messages_from_net(UDPNetHandler *nh, UDPC
nh->udp_callbacks.enqueue(uc);
uc->onCallbackQueue = 1;
}
+
+ return return_val == static_cast<int>(MAX_RECEIVE_MSG_PER_CALL);
}
#endif
void
UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection
*xuc)
{
#if HAVE_RECVMMSG
- read_multiple_messages_from_net(nh, xuc);
+ while (read_multiple_messages_from_net(nh, xuc)) {}
#else
Review Comment:
`udp_read_from_net()` now drains the socket with an unbounded `while
(read_multiple_messages_from_net(...)) {}` loop. Under sustained high UDP load
(e.g., QUIC), this can monopolize the net thread and starve other
connections/events because it keeps reading as long as the kernel returns
`MAX_RECEIVE_MSG_PER_CALL`.
Consider bounding the number of recvmmsg batches per event (or
yielding/scheduling) to preserve fairness, while still getting the benefits of
batching.
##########
tests/gold_tests/h3/h3_go_client.test.py:
##########
@@ -0,0 +1,128 @@
+'''
+Verify HTTP/3 client interop with a quic-go client.
+'''
+# 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
+
+Test.Summary = '''
+Verify that a quic-go HTTP/3 client can complete sequential and concurrent
+transactions through ATS.
+'''
+
+Test.SkipUnless(
+ Condition.HasATSFeature('TS_USE_QUIC'),
+ Condition.HasGoVersion('1.24'),
+)
+
+
+def add_default_ssl_multicert(ts):
+ """Configure the default server certificate."""
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+
+class TestHttp3GoClient:
+ """Configure a test to verify HTTP/3 quic-go client interoperability."""
+
+ replay_file = "replays/h3_server_for_go_client.replay.yaml"
+
+ def __init__(self, name: str):
+ """Initialize the test."""
+ self.name = name
+ self._configure_server()
+ self._configure_traffic_server()
+ self._configure_client()
+
+ def _configure_server(self):
+ """Configure the Proxy Verifier origin server."""
+ self._server = Test.MakeVerifierServerProcess("server-go-h3-client",
self.replay_file, verbose=False)
+
+ def _configure_traffic_server(self):
+ """Configure Traffic Server."""
+ ts = Test.MakeATSProcess("ts-go-h3-client", enable_tls=True,
enable_quic=True, enable_cache=False)
+ ts.addDefaultSSLFiles()
+ add_default_ssl_multicert(ts)
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'quic|http3',
+ 'proxy.config.quic.initial_max_data_in': 1000000,
+ 'proxy.config.quic.initial_max_stream_data_bidi_remote_in':
1000000,
+ 'proxy.config.quic.server.stateless_retry_enabled': 0,
+ 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir,
+ 'proxy.config.ssl.server.private_key.path':
ts.Variables.SSLDir,
+ })
+ ts.Disk.remap_config.AddLine(f'map /
http://127.0.0.1:{self._server.Variables.http_port}')
+ ts.Disk.logging_yaml.AddLines(
+ '''
+logging:
+ formats:
+ - name: h3_go_access
+ format: 'c_alpn=%<cqssa> client_version=%<cqpv> c_method=%<cqhm>
c_url=%<cquuc>'
+
+ logs:
+ - filename: h3_go_access
+ format: h3_go_access
+'''.split("\n"))
+
+ self._access_log = Test.Disk.File(os.path.join(ts.Variables.LOGDIR,
'h3_go_access.log'), exists=True)
+ self._access_log.Content = Testers.ContainsExpression(
+ r'c_alpn=h3 client_version=http/3 c_method=GET
c_url=https://go\.example\.com:[0-9]+/go-get-empty',
+ "ATS should log the quic-go request as HTTP/3")
+ self._access_log.Content += Testers.ContainsExpression(
+ r'c_alpn=h3 client_version=http/3 c_method=POST
c_url=https://go\.example\.com:[0-9]+/go-post-large',
+ "ATS should log the quic-go large POST as HTTP/3")
+
+ self._ts = ts
+
+ def _configure_client(self):
+ """Configure the quic-go client test runs."""
+ tr = Test.AddTestRun(self.name)
+ tr.Setup.Copy("go_h3_client")
+ tr.Processes.Default.StartBefore(self._server)
+ tr.Processes.Default.StartBefore(self._ts)
+ tr.Processes.Default.Env['GOFLAGS'] = '-mod=readonly'
+ tr.Processes.Default.Env['GOCACHE'] = os.path.join(tr.RunDirectory,
'gocache')
+ tr.Processes.Default.Env['GOMODCACHE'] = os.path.join(tr.RunDirectory,
'gomodcache')
+ tr.Processes.Default.Env['GOTOOLCHAIN'] = 'local'
+ tr.Processes.Default.Command = (
+ f'cd "{os.path.join(tr.RunDirectory, "go_h3_client")}" && '
+ f'go run . --addr 127.0.0.1:{self._ts.Variables.ssl_port} '
+ f'--authority go.example.com:{self._ts.Variables.ssl_port} '
+ '--server-name go.example.com')
Review Comment:
This test runs `go run` on a module that depends on external modules
(quic-go, x/*). Without a vendored module tree (or a pre-warmed module cache),
`go run` will attempt to download dependencies from the network, which is often
blocked/unreliable in CI and makes AuTests non-hermetic.
Consider vendoring the module (`go mod vendor`) and running with
`-mod=vendor` (or otherwise ensuring dependencies are available offline).
##########
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:
The new loop stops after the first transaction if it remains the list head,
so additional concurrent streams may never receive the close/timeout event and
may leak resources or miss teardown. This can happen whenever the first
transaction does not remove itself synchronously in handleEvent(). Iterate
safely over the whole list (capturing `next` before calling handleEvent)
instead of breaking on `head == t`.
--
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]
