Copilot commented on code in PR #13834:
URL: https://github.com/apache/trafficserver/pull/13834#discussion_r4264230398
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -122,6 +122,26 @@ def _wait_for_close(self, tls_socket: ssl.SSLSocket) ->
None:
print("drained_session_closed_by_peer", flush=True)
return
+ def finish_connection(self, tls_socket: ssl.SSLSocket) -> None:
+ """Half-close and read until ATS closes, so that closing cannot reset
the connection.
+
+ Closing a socket that still holds unread input, such as a SETTINGS ACK
+ that ATS sent after the last read, makes the kernel reset the
connection,
+ which can discard what this origin just wrote, such as the retried
response.
+ """
+ try:
+ tls_socket.shutdown(socket.SHUT_WR)
+ except OSError:
+ return
+ while not self.lifecycle.stopped:
+ try:
+ if not tls_socket.recv(65535):
+ return
+ except TimeoutError:
+ continue
+ except OSError:
+ return
Review Comment:
`finish_connection()` uses `SSLSocket.recv()` but doesn’t handle TLS EOF /
SSL-layer exceptions (e.g., `ssl.SSLZeroReturnError`, `ssl.SSLEOFError`, or
more general `ssl.SSLError`). `_wait_for_close()` already treats these as a
clean close; here they would escape the thread and can surface as noisy
unhandled exceptions (and potentially affect test stability). Align the
exception handling with `_wait_for_close()` by treating TLS EOF as `data = b''`
/ returning, and optionally handling `ConnectionResetError` similarly.
##########
tests/gold_tests/h2/safe_retry_origin.py:
##########
@@ -253,19 +278,25 @@ def serve_socket(plain_socket: socket.socket, listener:
socket.socket | None) ->
# Ignore readiness probes and peers that never finish TLS.
plain_socket.close()
return
- with tls_socket:
+ try:
if tls_socket.selected_alpn_protocol() != "h2":
raise RuntimeError("ATS did not negotiate HTTP/2 with the
origin")
tls_socket.settimeout(0.2)
origin.serve_connection(tls_socket, listener)
+ except BaseException:
+ tls_socket.close()
+ raise
+ # Finish in the background: ATS can keep a finished connection open
+ # while it opens the next one, which this origin must accept.
+ threads.append(threading.Thread(target=finish_socket,
args=(tls_socket,)))
Review Comment:
These finisher threads are created as non-daemon threads by default. If
`run_server()` shutdown does not reliably join all threads in `threads`, the
process can hang waiting for finisher threads (especially if ATS keeps the
connection open until near shutdown). Ensure these finisher threads are either
marked daemon or are always joined during shutdown (including in error paths).
--
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]
