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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
