Copilot commented on code in PR #13695:
URL: https://github.com/apache/trafficserver/pull/13695#discussion_r4137801572
##########
src/proxy/http2/HTTP2.cc:
##########
@@ -45,6 +45,28 @@ struct Http2HeaderName {
static VersionConverter hvc;
+void
+establish_flow_control_policy(const char *name,
std::atomic<Http2FlowControlPolicy> &policy)
+{
+ auto update = [](const char *name, RecDataT type, RecData data, void
*cookie) -> int {
Review Comment:
This lambda has an empty capture list but refers to the enclosing `name`
parameter on line 56, so `HTTP2.cc` will not compile. Capture the record name
(and omit or rename the callback's first parameter) before registering the
callback.
##########
tests/gold_tests/h2/http2_flow_control_reload.py:
##########
@@ -0,0 +1,142 @@
+"""Probe HTTP/2 receive windows while reloading flow control policies."""
+
+# 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 concurrent.futures
+import http.client
+import socket
+import ssl
+import subprocess
+import time
+
+from h2.config import H2Configuration
+from h2.connection import H2Connection
+from h2.events import PingAckReceived, RequestReceived
+
+
+class WindowProbe:
+ """Measure ATS receive windows from each side of a fresh connection."""
+
+ def __init__(self, args: argparse.Namespace) -> None:
+ self._args = args
+ self._client_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
+ self._client_context.check_hostname = False
+ self._client_context.verify_mode = ssl.CERT_NONE
+ self._client_context.set_alpn_protocols(['h2'])
+ self._server_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
+ self._server_context.load_cert_chain(args.cert, args.key)
+ self._server_context.set_alpn_protocols(['h2'])
+
+ def _window(self, sock: ssl.SSLSocket, is_client: bool) -> int:
+ assert sock.selected_alpn_protocol() == 'h2', 'Expected HTTP/2'
+ connection =
H2Connection(config=H2Configuration(client_side=is_client))
+ connection.initiate_connection()
+ connection.ping(b'reload!!')
+ sock.sendall(connection.data_to_send())
+ # The PING acknowledgement is a barrier: ATS has sent its initial
+ # SETTINGS and connection WINDOW_UPDATE before acknowledging our PING.
+ acknowledged = False
+ received_request = is_client
+ while not (acknowledged and received_request):
+ data = sock.recv(65536)
+ assert data, 'ATS closed the connection before the probe completed'
+ for event in connection.receive_data(data):
+ if isinstance(event, PingAckReceived):
+ acknowledged = True
+ elif isinstance(event, RequestReceived):
+ received_request = True
+ connection.send_headers(event.stream_id, [(':status',
'200'), ('content-length', '0')], end_stream=True)
+ sock.sendall(connection.data_to_send())
+ return connection.outbound_flow_control_window
+
+ def inbound(self) -> int:
+ with socket.create_connection(('127.0.0.1', self._args.https_port),
timeout=5) as raw:
+ with self._client_context.wrap_socket(raw,
server_hostname='localhost') as sock:
+ return self._window(sock, True)
+
+ def _request(self) -> None:
+ connection = http.client.HTTPConnection('127.0.0.1',
self._args.http_port, timeout=5)
+ try:
+ connection.request('GET', '/', headers={'Connection': 'close'})
+ response = connection.getresponse()
+ assert response.status == 200, f'Unexpected origin response:
{response.status}'
+ response.read()
+ finally:
+ connection.close()
+
+ def outbound(self) -> int:
+ with socket.socket() as listener:
+ listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+ listener.bind(('127.0.0.1', self._args.origin_port))
+ listener.listen(1)
+ listener.settimeout(5)
+ with concurrent.futures.ThreadPoolExecutor(max_workers=1) as
executor:
+ request = executor.submit(self._request)
+ raw, _ = listener.accept()
+ with raw:
+ raw.settimeout(5)
+ with self._server_context.wrap_socket(raw,
server_side=True) as sock:
+ window = self._window(sock, False)
+ request.result(timeout=5)
+ return window
+
+ def expect(self, inbound_policy: int, outbound_policy: int, wait: bool =
False) -> None:
+ expected = tuple(65535 if policy == 0 else 6553500 for policy in
(inbound_policy, outbound_policy))
+ deadline = time.monotonic() + (20 if wait else 0)
+ while True:
+ actual = (self.inbound(), self.outbound())
+ if actual == expected:
+ print(f'policies in={inbound_policy}, out={outbound_policy}:
windows={actual}', flush=True)
+ return
+ if time.monotonic() >= deadline:
+ raise AssertionError(
+ f'policies in={inbound_policy}, out={outbound_policy}:
expected windows {expected}, got {actual}')
+ time.sleep(0.25)
+
+ def run(self) -> None:
+ self.expect(0, 0)
+ # Reset to policy 0 before policy 2 so the large window proves that
+ # both nonzero policy values actually take effect. Probe the opposite
+ # direction too, so crossed or shared callbacks cannot pass.
+ for direction in ('in', 'out'):
+ record = f'proxy.config.http2.flow_control.policy_{direction}'
+ for policy in (1, 0, 2, 0):
+ subprocess.run(['traffic_ctl', 'config', 'set', record,
str(policy)], check=True, timeout=10)
+ self.expect(policy if direction == 'in' else 0, policy if
direction == 'out' else 0, wait=True)
Review Comment:
Both policy 1 and policy 2 produce the same expected connection window here,
so this regression test cannot distinguish a runtime update to policy 2 from
policy 1. The behavior unique to policy 2 is dynamic stream-window adjustment;
an implementation that clamps 2 to 1 would pass every assertion. Add a probe
that exercises multiple streams after reloading policy 2 and verifies the
per-stream behavior before switching back.
--
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]
