Copilot commented on code in PR #13198:
URL: https://github.com/apache/trafficserver/pull/13198#discussion_r3293827358
##########
tests/gold_tests/autest-site/ports.py:
##########
@@ -236,6 +236,45 @@ def _get_port_by_bind():
return port
+def _reserve_port():
+ """
+ Get a port from the global port queue.
+
+ Returns:
+ A tuple containing the port value and whether it should be recycled
+ into the queue when the caller is done with it.
+ """
+ _setup_port_queue()
+ if g_ports.qsize() > 0:
+ try:
+ port = _get_available_port(g_ports)
+ host.WriteVerbose("_reserve_port", f"Using port from port queue:
{port}")
+ return port, True
+ except PortQueueSelectionError:
+ port = _get_port_by_bind()
+ host.WriteVerbose("_reserve_port", f"Queue was drained. Using port
from a bound socket: {port}")
+ return port, False
+
+ # Since the queue could not be populated, use a port via bind.
+ port = _get_port_by_bind()
+ host.WriteVerbose("_reserve_port", f"Queue is empty. Using port from a
bound socket: {port}")
+ return port, False
+
+
+def get_port_number():
+ """
+ Get a port number from the same allocator used by get_port().
+
+ This is useful for helper code that needs a temporary listening port but
+ does not have an AuTest object with Setup hooks for recycling it.
+
+ Returns:
+ A port value.
+ """
+ port, _ = _reserve_port()
+ return port
Review Comment:
get_port_number() pulls from the global port queue via _reserve_port() but
never recycles the port when it came from the queue. Over many condition
evaluations this will drain g_ports and force later callers (including
get_port()) into the bind-based fallback, increasing the chance of port
races/flaky tests. Consider making get_port_number() avoid the queue (always
use _get_port_by_bind), or change the API to return a recyclable reservation
(e.g., a context manager / (port, recycle_fn)) so callers can put the port back
when done.
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -20,6 +20,107 @@ import os
import subprocess
import json
import re
+import tempfile
+import time
+
+from ports import get_port_number
+
+OPENSSL_TLS_FLAGS = {
+ "1.0": "-tls1",
+ "1.1": "-tls1_1",
+ "1.2": "-tls1_2",
+ "1.3": "-tls1_3",
+}
+
+
+def _terminate_process(process):
+ if process.poll() is not None:
+ return
+ process.terminate()
+ try:
+ process.wait(timeout=2)
+ except subprocess.TimeoutExpired:
+ process.kill()
+ process.wait(timeout=2)
+
+
+def _probe_openssl_server(tls_version, client_probe):
+ """Run a local OpenSSL server for TLS capability probes.
+
+ This owns the temporary certificate, port allocation, and server process
+ lifecycle so AuTest conditions can perform a real handshake with the client
+ being checked. Local setup failures return ``False`` so callers can skip
+ dependent tests instead of failing the harness.
+
+ :param tls_version: TLS version string to look up in
+ ``OPENSSL_TLS_FLAGS``.
+ :param client_probe: Callable that receives the server port and TLS flag
+ and returns whether the client completed the expected handshake.
+ :returns: ``True`` if the client probe succeeds against the local server,
+ otherwise ``False``.
+ """
+ tls_flag = OPENSSL_TLS_FLAGS.get(tls_version)
+ if tls_flag is None:
+ return False
+
+ try:
+ with tempfile.TemporaryDirectory() as tmpdir:
+ cert_path = os.path.join(tmpdir, "cert.pem")
+ key_path = os.path.join(tmpdir, "key.pem")
+ result = subprocess.run(
+ [
+ "openssl",
+ "req",
+ "-x509",
+ "-newkey",
+ "rsa:2048",
+ "-nodes",
+ "-sha256",
+ "-keyout",
+ key_path,
+ "-out",
+ cert_path,
+ "-subj",
+ "/CN=localhost",
+ "-days",
+ "1",
+ ],
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ timeout=10,
+ )
+ if result.returncode != 0:
+ return False
+
+ port = get_port_number()
+ server = subprocess.Popen(
+ [
+ "openssl",
+ "s_server",
Review Comment:
_probe_openssl_server() uses get_port_number(), which currently may consume
a port from the shared g_ports queue without recycling it. Because
_probe_openssl_server owns the server lifecycle, it should reserve a port in a
way that can be returned to the queue after the probe completes (or ensure
get_port_number() never takes from the queue) to avoid draining the global
allocator over the full test run.
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -20,6 +20,107 @@ import os
import subprocess
import json
import re
+import tempfile
+import time
+
+from ports import get_port_number
+
+OPENSSL_TLS_FLAGS = {
+ "1.0": "-tls1",
+ "1.1": "-tls1_1",
+ "1.2": "-tls1_2",
+ "1.3": "-tls1_3",
+}
+
+
+def _terminate_process(process):
+ if process.poll() is not None:
+ return
+ process.terminate()
+ try:
+ process.wait(timeout=2)
+ except subprocess.TimeoutExpired:
+ process.kill()
+ process.wait(timeout=2)
Review Comment:
_terminate_process() can still raise subprocess.TimeoutExpired on the second
wait() after kill(), which then relies on outer callers catching it (and may
skip cleanup/reaping). It would be safer to catch TimeoutExpired (and possibly
OSError) for the kill/wait path and swallow it so probe cleanup is best-effort
and doesn’t leave zombies behind.
--
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]
