maskit commented on code in PR #13641:
URL: https://github.com/apache/trafficserver/pull/13641#discussion_r3929035889
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -149,6 +153,7 @@ Http2ConnectionState::rcv_data_frame(const Http2Frame
&frame)
// the recipient MUST respond with a stream error of type STREAM_CLOSED.
if (stream->get_state() != Http2StreamState::HTTP2_STREAM_STATE_OPEN &&
stream->get_state() !=
Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL) {
+ this->credit_discarded_data(payload_length);
Review Comment:
**1. Same-class discard paths further down are still uncredited.**
Two later returns in this function discard the payload for ATS's *own*
internal reasons with `HTTP2_ERROR_INTERNAL_ERROR` — the `writer == nullptr`
case and the "Write mismatch" case. The peer did nothing wrong in either, so
not crediting those desyncs an honest peer's connection window in the same way
this PR is fixing. `payload_length_is_valid()` failure has the same shape but
is the peer's fault, so it's lower priority, and it's bounded by
`stream_error_count` once `stream_requests >= 10`.
Reasonable to leave for a follow-up, but they look like the same bug.
FWIW I also checked the `change_state()` failure branch a few lines below,
which also sends `RST_STREAM` and returns `CLASS_NONE`, since it looked like a
fourth gap. It's **dead code**: at that point the state is necessarily `OPEN`
or `HALF_CLOSED_LOCAL` per the check right above, and `change_state(DATA,
END_STREAM)` with `receive_end_stream == true` returns true for both. So no
credit is needed there.
##########
include/proxy/http2/Http2ConnectionState.h:
##########
@@ -130,6 +130,7 @@ class Http2ConnectionState : public Continuation
void release_stream();
void cleanup_streams();
void restart_receiving(Http2Stream *stream);
+ void credit_discarded_data(uint32_t payload_length);
Review Comment:
**2. This can be private, and the parameter is doing less than it looks.**
Nothing outside `Http2ConnectionState.cc` calls it — unlike
`restart_receiving()`, which is public because `Http2Stream::reenable()` needs
it. Suggest moving it to the private section.
Also, `credit_discarded_data()` doesn't credit `payload_length`:
`restart_receiving()` is an absolute top-up to `configured_session_window`, so
the argument is used only in the debug line. Either rename to reflect the
top-up semantics, or drop the parameter and log `payload_length` at the call
site.
##########
tests/gold_tests/h2/http2_flow_control.test.py:
##########
@@ -383,6 +383,47 @@ def run(self) -> None:
'ATS should log the expected SETTINGS_TIMEOUT connection error.')
+class Http2ClosedStreamFlowControlTest:
+ """Verify DATA to closed streams cannot drain the connection window."""
+
+ def run(self) -> None:
+ """Configure the test run."""
+ tr = Test.AddTestRun('Closed-stream DATA does not drain connection
window')
+ server = tr.MakeHttpBinServer('server-closed-stream-window')
+ ts = tr.MakeATSProcess('ts-closed-stream-window', enable_tls=True,
enable_cache=False)
+
+ ts.addDefaultSSLFiles()
+ ts.Setup.CopyAs('clients/h2_closed_stream_window_drain.py',
Test.RunDirectory)
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http2',
+ 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path':
f'{ts.Variables.SSLDir}',
+ })
+ ts.Disk.remap_config.AddLine(f'map /
http://127.0.0.1:{server.Variables.Port}')
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ tr.Processes.Default.StartBefore(server)
+ tr.Processes.Default.StartBefore(ts)
+ tr.Processes.Default.Command = f'{sys.executable}
h2_closed_stream_window_drain.py {ts.Variables.ssl_port}'
+ tr.Processes.Default.ReturnCode = 0
Review Comment:
**5. Consider an explicit `tr.TimeOut`.**
`Http2FlowControlTest` in this file sets `tr.TimeOut = 20`. On the failure
path this client can stack several timeouts — a 3 s
`WINDOW_UPDATE_TIMEOUT_SECONDS` wait plus 10 s default socket timeouts in
`wait_for_response()` — so an explicit bound would make a real regression fail
fast and legibly instead of hitting the framework default.
##########
tests/gold_tests/h2/http2_flow_control.test.py:
##########
@@ -383,6 +383,47 @@ def run(self) -> None:
'ATS should log the expected SETTINGS_TIMEOUT connection error.')
+class Http2ClosedStreamFlowControlTest:
Review Comment:
**6. Outbound direction is fixed but untested.**
The `credit_discarded_data()` call on the `stream == nullptr` path sits
above the `is_outbound()` split, so ATS-as-client gets the fix too. That's the
more interesting direction, since outbound H2 sessions are pooled and
multiplexed, so a drained connection window there can affect more than one
transaction. But there's no coverage for it. `Http2FlowControlTest` in this
file already has the outbound plumbing (`AddVerifierServerProcess` with an
HTTP/2 origin) if you want a case here; otherwise a follow-up seems fine.
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -126,6 +128,7 @@ Http2ConnectionState::rcv_data_frame(const Http2Frame
&frame)
if (stream == nullptr) {
if (this->is_valid_streamid(id)) {
// This error occurs fairly often, and is probably innocuous (SM
initiates the shutdown)
+ this->credit_discarded_data(payload_length);
Review Comment:
**3. The new call splits a comment from its subject.**
The pre-existing `// This error occurs fairly often, and is probably
innocuous (SM initiates the shutdown)` belongs to the RST / stream-error
decision below it, but now reads as if it annotates `credit_discarded_data()`.
Moving the new call above the comment keeps it attached to what it describes.
##########
tests/gold_tests/h2/clients/h2_closed_stream_window_drain.py:
##########
@@ -0,0 +1,291 @@
+#!/usr/bin/env python3
+'''
+HTTP/2 client that floods a closed stream with DATA and then verifies the
+connection is still usable.
+'''
+# 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 argparse
+import socket
+import ssl
+from typing import Dict, List, Optional, Tuple
+
+import hpack
+
+CONNECTION_PREFACE = b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'
+
+FRAME_TYPE_DATA = 0
+FRAME_TYPE_HEADERS = 1
+FRAME_TYPE_RST_STREAM = 3
+FRAME_TYPE_SETTINGS = 4
+FRAME_TYPE_PING = 6
+FRAME_TYPE_GOAWAY = 7
+FRAME_TYPE_WINDOW_UPDATE = 8
+
+FLAG_ACK = 0x01
+FLAG_END_STREAM = 0x01
+FLAG_END_HEADERS = 0x04
+
+CONNECTION_STREAM_ID = 0
+CLOSED_STREAM_ID = 1
+PROBE_STREAM_ID = 3
+
+MAX_FRAME_SIZE = 16384
Review Comment:
**4. Hardcoded peer settings, and the "honest client" claim is
connection-level only.**
The client never reads these values out of the SETTINGS frame it already
receives and ACKs. Both constants match current ATS defaults, so the test
passes today — but if `proxy.config.http2.max_frame_size` ever changes, this
surfaces as a confusing `FRAME_SIZE_ERROR` rather than a clear failure. Parsing
them from the SETTINGS payload is cheap here.
Related: the client tracks only the *connection* send window, so it pushes
`2 * 65535` bytes at stream 1 whose *stream* send window is 65535. ATS never
checks the stream window on the discard path, so the test works as intended —
but the comment just below ("An honest client can only do this if...")
overstates it. Worth saying connection-level explicitly.
--
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]
