Copilot commented on code in PR #13873:
URL: https://github.com/apache/trafficserver/pull/13873#discussion_r4340660284
##########
tests/uranium_tests/h2/h2active_timeout.py:
##########
@@ -54,16 +54,28 @@ def get_socket(port: int) -> socket.socket:
return tls_socket
-def makerequest(port: int, path: str, delay: int) -> None:
- """Establish an HTTP/2 connection and send a request.
+def makerequest(port: int, path: str, active_timeout: int, max_wait: int) ->
None:
+ """Send a request, then wait for ATS to close the connection on its active
timeout.
+
+ ATS enforces the active timeout from its InactivityCop, which looks at a
+ connection only once a second, so the close lands up to a second after the
+ timeout expires, and later still on a loaded machine. Rather than guess
when
+ that is, wait for the close itself and check that it came no sooner than
the
+ timeout and within max_wait seconds.
:param port: The port to connect to.
:param path: The path to request.
- :param delay: The delay to wait between sending requests in a stream.
+ :param active_timeout: The proxy.config.http2.active_timeout_in ATS uses,
in seconds.
+ :param max_wait: The number of seconds to wait for ATS to close the
connection.
"""
tls_socket = get_socket(port)
+ # ATS starts the active timeout when it reads the connection preface, which
+ # happens after this point, so it cannot close the connection any sooner
+ # than active_timeout seconds from now.
+ start = time.monotonic()
+
h2_connection = h2.connection.H2Connection()
h2_connection.initiate_connection()
tls_socket.sendall(h2_connection.data_to_send())
Review Comment:
The timer starts before the client sends the HTTP/2 connection preface (line
77 vs 81), which can overestimate `elapsed` relative to when ATS actually
begins its active-timeout accounting. That can misclassify an early close
(relative to ATS’s preface-read time) as on-time if there’s any noticeable
delay between `start` and the preface being read. Consider moving `start =
time.monotonic()` to immediately after the preface is sent (or after the first
write that includes the preface), and update the comment accordingly so the
measurement aligns with ATS’s timeout start.
##########
tests/uranium_tests/h2/h2active_timeout.py:
##########
@@ -78,65 +90,46 @@ def makerequest(port: int, path: str, delay: int) -> None:
h2_connection.send_headers(1, headers, end_stream=True)
tls_socket.sendall(h2_connection.data_to_send())
- # delay, triggering ATS timeout.
- time.sleep(delay)
-
- # The following should fail due to the timeout.
+ # Read the response, then keep reading until ATS closes the connection.
+ closed = False
try:
- # Send a second request.
- h2_connection.send_headers(3, headers, end_stream=True)
- tls_socket.sendall(h2_connection.data_to_send())
-
- response_stream_ended = False
- body = b''
- while not response_stream_ended:
- # read raw data from the socket
- data = tls_socket.recv(65536 * 1024)
+ while True:
+ remaining = start + max_wait - time.monotonic()
+ if remaining <= 0:
+ break
Review Comment:
At the exact `max_wait` boundary, the loop breaks without attempting a final
read, which can misreport a connection as 'still open' if the close arrives
right as `remaining` crosses zero. A small final non-blocking read (or a brief
epsilon grace window) would make the boundary behavior more deterministic and
easier to reason about.
##########
tests/uranium_tests/h2/h2active_timeout.py:
##########
@@ -78,65 +90,46 @@ def makerequest(port: int, path: str, delay: int) -> None:
h2_connection.send_headers(1, headers, end_stream=True)
tls_socket.sendall(h2_connection.data_to_send())
- # delay, triggering ATS timeout.
- time.sleep(delay)
-
- # The following should fail due to the timeout.
+ # Read the response, then keep reading until ATS closes the connection.
+ closed = False
try:
- # Send a second request.
- h2_connection.send_headers(3, headers, end_stream=True)
- tls_socket.sendall(h2_connection.data_to_send())
-
- response_stream_ended = False
- body = b''
- while not response_stream_ended:
- # read raw data from the socket
- data = tls_socket.recv(65536 * 1024)
+ while True:
+ remaining = start + max_wait - time.monotonic()
+ if remaining <= 0:
+ break
+ tls_socket.settimeout(remaining)
+ data = tls_socket.recv(65536)
if not data:
+ closed = True
break
-
- # feed raw data into h2, and process resulting events
- events = h2_connection.receive_data(data)
- for event in events:
- if isinstance(event, h2.events.ResponseReceived):
- # response headers received
- print("Response received:")
- for header in event.headers:
- print(f' {header[0].decode()}: {header[1].decode()}')
+ for event in h2_connection.receive_data(data):
if isinstance(event, h2.events.DataReceived):
- # update flow control so the server doesn't starve us
h2_connection.acknowledge_received_data(event.flow_controlled_length,
event.stream_id)
- # more response body data received
- body += event.data
- if isinstance(event, h2.events.StreamEnded):
- # response body completed, let's exit the loop
- response_stream_ended = True
- break
- # send any pending data to the server
tls_socket.sendall(h2_connection.data_to_send())
Review Comment:
Setting the socket timeout (line 100) applies to both `recv` and the
subsequent `sendall` (line 108). Near the deadline, `sendall` may raise
`socket.timeout`, which is currently treated the same as a read timeout (caught
and ignored), potentially masking errors and reporting 'still open' when the
loop exits. To avoid this, limit the short timeout to the read operation (e.g.,
save the prior timeout and restore it before `sendall`, or only call `sendall`
when the socket is in a known-safe timeout 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
