Copilot commented on code in PR #18058:
URL: https://github.com/apache/pinot/pull/18058#discussion_r3036158333
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -477,9 +480,24 @@ public void testSkipUnavailableServer()
dataTableMetadata.put(MetadataKey.REQUEST_ID.getName(),
Long.toString(requestId));
byte[] successResponseBytes = dataTableSuccess.toBytes();
- // Only start a single QueryServer, on port from serverInstance1
- _queryServer = getQueryServer(500, successResponseBytes, port);
- _queryServer.start();
+ // Start server1 on port 0 (OS-assigned) to avoid TOCTOU race
+ _queryServer = getQueryServer(500, successResponseBytes, 0);
+ int port1 = startAndGetPort(_queryServer);
+
+ // For server2 (unavailable server), use port 1 which requires root to
bind and is
+ // guaranteed to have nothing listening in test environments. This avoids
TOCTOU races
+ // and gives an immediate "connection refused" (no timeout delay).
+ int port2 = 1;
Review Comment:
Using `port2 = 1` to represent an unavailable server is not a reliable
guarantee that nothing is listening: privileged processes can still bind to
port 1, which could cause the test to connect successfully and become
flaky/hang. Prefer a deterministically-unlistenable port (e.g., `port2 = 0`,
which cannot be bound by any process) or another approach that guarantees fast
connection failure without relying on environment assumptions.
```suggestion
// For server2 (unavailable server), use port 0 because it cannot
represent a real
// listening endpoint, avoiding environment-specific assumptions about
privileged ports.
int port2 = 0;
```
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -85,6 +80,7 @@ public void setUp() {
@AfterMethod
void shutdownServer() {
if (_queryServer != null) {
+ // shutDown() blocks on channel.close().sync(), so port is released when
this returns
_queryServer.shutDown();
_queryServer = null;
}
Review Comment:
`shutdownServer()` unconditionally calls `_queryServer.shutDown()`, but
`QueryServer.start()` can throw before `_channel` is assigned (e.g., bind
failure), leaving `_channel` null. In that case `shutDown()` will NPE on
`_channel.close()`, masking the real test failure. Consider guarding teardown
with `if (_queryServer.getChannel() != null)` (or nulling `_queryServer` when
`startAndGetPort()` fails) before calling `shutDown()`.
```suggestion
if (_queryServer == null) {
return;
}
// A failed start can leave the server without an initialized channel.
if (_queryServer.getChannel() != null) {
// shutDown() blocks on channel.close().sync(), so port is released
when this returns
_queryServer.shutDown();
}
_queryServer = null;
```
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -106,6 +112,16 @@ private QueryServer getQueryServer(int responseDelayMs,
byte[] responseBytes, in
return new QueryServer(port, null, handler);
}
+ /**
+ * Starts the query server and returns the actual bound port. Uses port 0 to
let the OS assign a free port,
+ * avoiding TOCTOU race conditions. {@link QueryServer#start()} blocks on
{@code bind().sync()}, so the
+ * server is ready to accept connections when this method returns.
+ */
+ private int startAndGetPort(QueryServer server) {
+ server.start();
+ return ((InetSocketAddress) server.getChannel().localAddress()).getPort();
+ }
Review Comment:
PR description mentions adding `startQueryServerWithWait()` and a shutdown
"port release delay" + exception handling, but the code introduces
`startAndGetPort()` and teardown still directly calls `shutDown()` (no delay,
and no extra exception handling beyond the new comment). Either update the PR
description to match the implemented approach, or implement the described
helpers/behavior so the PR narrative matches the diff.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
