Copilot commented on code in PR #13873:
URL: https://github.com/apache/trafficserver/pull/13873#discussion_r4332590158
##########
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())
-
- print(f"Response fully received: {len(body)} bytes")
-
- body_str = body.decode('utf-8')
-
- # tell the server we are closing the h2 connection
- h2_connection.close_connection()
- tls_socket.sendall(h2_connection.data_to_send())
-
- # close the socket
- tls_socket.close()
- except Exception:
+ except socket.timeout:
+ pass
+ except (ConnectionError, ssl.SSLError):
+ closed = True
+ elapsed = time.monotonic() - start
+ tls_socket.close()
+
+ if not closed:
+ print(f"Connection still open after {max_wait} seconds")
+ elif elapsed < active_timeout:
+ print(f"Connection closed after {elapsed:.1f} seconds, before the
{active_timeout} second active timeout")
+ else:
print("CONNECTION_TIMEOUT")
Review Comment:
On failure outcomes (`not closed` or `elapsed < active_timeout`), the script
prints a message but still exits with status 0. Since this is a test helper,
returning a non-zero exit code for failure cases would make CI failures easier
to diagnose (and avoid depending solely on output substring checks).
##########
tests/uranium_tests/h2/test_http2.py:
##########
@@ -198,8 +200,9 @@ def test_http2_active_timeout(ats_factory: ATSFactory,
services: ServiceFactory)
TEST_DIRECTORY / "h2active_timeout.py",
str(_ats.https_port),
"/",
- "4",
- timeout=10,
+ str(ACTIVE_TIMEOUT),
+ # Longer than the 15 seconds h2active_timeout.py waits for the close.
+ timeout=30,
Review Comment:
The test assumes `h2active_timeout.py` waits 15 seconds, but it currently
relies on that script’s default (`--max_wait` default=15). To avoid this
becoming brittle if the script default changes, consider passing `--max_wait`
explicitly from the test and deriving the subprocess `timeout` from that value
(+ small slack).
##########
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())
-
- print(f"Response fully received: {len(body)} bytes")
-
- body_str = body.decode('utf-8')
-
- # tell the server we are closing the h2 connection
- h2_connection.close_connection()
- tls_socket.sendall(h2_connection.data_to_send())
-
- # close the socket
- tls_socket.close()
- except Exception:
+ except socket.timeout:
+ pass
+ except (ConnectionError, ssl.SSLError):
+ closed = True
+ elapsed = time.monotonic() - start
+ tls_socket.close()
Review Comment:
Socket cleanup isn’t guaranteed if an unexpected exception occurs inside the
read/parse loop (e.g., an `h2` protocol exception). Consider ensuring
`tls_socket.close()` runs via a `finally` (or a context manager) so this helper
doesn’t leak resources on error paths.
##########
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:
The updated check can print `CONNECTION_TIMEOUT` even if the initial request
never actually received a response (e.g., no `ResponseReceived`/`StreamEnded`
observed) as long as the socket closes after `active_timeout`. To avoid false
positives, track that stream 1 received a response (at least
`ResponseReceived`, ideally `StreamEnded`) and treat ‘no response observed
before close/timeout’ as a failure path.
--
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]
