Skip to content

Commit e8c2dcd

Browse files
committed
fix(qwp): cancel in-flight connect during close, with a bounded backstop
CursorWebSocketSendLoop.close() did an untimed shutdownLatch.await() while the I/O thread could be blocked in an in-flight foreground connect (connect_timeout=0 => OS SYN-retry ~60-130s). The connecting WebSocketClient is a walk-local in QwpWebSocketSender.connectWalk, invisible to close() (the `client` field is null on async-initial connect / stale on reconnect), so closeTraffic() could not reach it and close() hung -- risking Sender.close() exceeding the sidecar's 120s deadline. Publish a race-safe per-loop cancellation handle (ConnectCancellation) through the transport seam: connectWalk publishes the client it is about to block on before connect(); close() calls closeTraffic() on it to unwind a black-holed connect. The ReconnectFactory seam gains a Java-8 `default reconnect(ConnectCancellation)`, so every existing implementor stays source/binary compatible. Add a bounded backstop: close() awaits the shutdown latch for at most DEFAULT_CLOSE_SHUTDOWN_AWAIT_MILLIS (30s, ~4x under the sidecar deadline) and, on timeout, runs the same loud failed-stop protocol as the interrupt branch (delegates final teardown to the I/O thread's exit path, frees nothing under the live worker) -- so close() returns bounded even in the rare TOCTOU window where cancellation is a no-op. Also clear the in-flight handle on connect-failure paths so it never dangles at a disposed client. Adds CursorWebSocketSendLoopConnectPhaseCloseTest (async-initial + mid-flight cancellation, plus bounded-backstop-without-cancellation).
1 parent 79eb13c commit e8c2dcd

3 files changed

Lines changed: 715 additions & 7 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2768,22 +2768,37 @@ private WebSocketClient newWebSocketClient() {
27682768
* reconnect and {@code close()} paths are therefore never queued
27692769
* behind a drainer's endpoint walk.
27702770
*/
2771-
private WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
2771+
private WebSocketClient buildAndConnect(ReconnectSupplier ctx, CursorWebSocketSendLoop.ConnectCancellation cancellation) {
27722772
if (ctx.isBackground()) {
27732773
// Lock-free: the walk below touches only internally-synchronized
27742774
// hostTracker health state and walk-local/cursor-local state on
27752775
// the background path.
2776-
return connectWalk(ctx);
2776+
return connectWalk(ctx, cancellation);
27772777
}
27782778
connectWalkLock.lock();
27792779
try {
2780-
return connectWalk(ctx);
2780+
return connectWalk(ctx, cancellation);
27812781
} finally {
27822782
connectWalkLock.unlock();
27832783
}
27842784
}
27852785

2786-
private WebSocketClient connectWalk(ReconnectSupplier ctx) {
2786+
// Drop the in-flight connect handle once the walk has disposed a client on
2787+
// a connect/upgrade FAILURE, so inFlight never dangles at a disposed client
2788+
// across the inter-attempt backoff. Without this a concurrent
2789+
// close()->cancel() could closeTraffic() a client the walk no longer owns;
2790+
// proven a no-op on both production transports today (closed-fd closeTraffic
2791+
// no-ops), but a future custom transport that threw on a closed socket would
2792+
// spuriously loud-fail close(). Null-guarded: the no-arg reconnect() path
2793+
// and the Unsafe.allocateInstance bare-loop tests pass a null handle. Pairs
2794+
// with the success-path clear() after upgrade().
2795+
private static void clearInFlight(CursorWebSocketSendLoop.ConnectCancellation cancellation) {
2796+
if (cancellation != null) {
2797+
cancellation.clear();
2798+
}
2799+
}
2800+
2801+
private WebSocketClient connectWalk(ReconnectSupplier ctx, CursorWebSocketSendLoop.ConnectCancellation cancellation) {
27872802
// Background (drainer) factories share this connect walk -- endpoint
27882803
// list and hostTracker HEALTH state (never the shared round: a
27892804
// background sweep walks its own RoundCursor and records with
@@ -2871,9 +2886,34 @@ private WebSocketClient connectWalk(ReconnectSupplier ctx) {
28712886
newClient.setQwpClientId(QwpConstants.CLIENT_ID);
28722887
newClient.setQwpRequestDurableAck(requestDurableAck);
28732888
newClient.setConnectTimeout(effectiveConnectTimeoutMs(background, connectTimeoutMs));
2889+
if (cancellation != null) {
2890+
// Publish the client we are about to block on so a
2891+
// concurrent CursorWebSocketSendLoop.close() can break its
2892+
// traffic and unwind a black-holed native connect
2893+
// (connect_timeout=0 => OS SYN-retry) instead of hanging on
2894+
// the untimed shutdown-latch await. The publish-then-check
2895+
// handshake pairs with ConnectCancellation.cancel(): if we
2896+
// observe cancellation here we skip the blocking connect
2897+
// entirely; otherwise cancel() observed this client and
2898+
// breaks it. The walk's per-attempt catch disposes the
2899+
// client and, since running has flipped false, the
2900+
// top-of-loop ctx.isAborted() gate ends the walk.
2901+
cancellation.publish(newClient);
2902+
if (cancellation.isCancelled()) {
2903+
throw new LineSenderException(ctx.abortMessage());
2904+
}
2905+
}
28742906
newClient.connect(ep.host, ep.port);
28752907
int upgradeTimeoutMs = (int) Math.min(authTimeoutMs, Integer.MAX_VALUE);
28762908
newClient.upgrade(WRITE_PATH, upgradeTimeoutMs, authorizationHeader);
2909+
if (cancellation != null) {
2910+
// connect()+upgrade() completed: this client is no longer
2911+
// blocking, so drop it from the in-flight handle before it
2912+
// becomes the loop's `client` field. close() must then break
2913+
// its traffic via the field path exactly once -- leaving it
2914+
// in the handle too would double-shut-down the socket.
2915+
cancellation.clear();
2916+
}
28772917
} catch (HttpClientException e) {
28782918
// Close BEFORE classify: the sibling catch (Error) below does not
28792919
// guard catch-arm bodies, so an Error thrown inside classify()
@@ -2883,6 +2923,7 @@ private WebSocketClient connectWalk(ReconnectSupplier ctx) {
28832923
// upgradeStatusCode) that are set during upgrade() and survive
28842924
// close().
28852925
newClient.close();
2926+
clearInFlight(cancellation);
28862927
HttpClientException classified = QwpUpgradeFailures.classify(newClient, ep.host, ep.port, e);
28872928
if (classified instanceof QwpIngressRoleRejectedException) {
28882929
QwpIngressRoleRejectedException re = (QwpIngressRoleRejectedException) classified;
@@ -2929,6 +2970,7 @@ private WebSocketClient connectWalk(ReconnectSupplier ctx) {
29292970
continue;
29302971
} catch (Exception e) {
29312972
newClient.close();
2973+
clearInFlight(cancellation);
29322974
hostTracker.recordTransportError(idx, !background);
29332975
lastError = e;
29342976
if (!background) {
@@ -2951,6 +2993,7 @@ private WebSocketClient connectWalk(ReconnectSupplier ctx) {
29512993
// the cursor reconnect loop, BackgroundDrainer) rethrows Error
29522994
// rather than retrying, so this stays a loud one-shot failure.
29532995
closeQuietlyOnError(newClient);
2996+
clearInFlight(cancellation);
29542997
throw e;
29552998
}
29562999
// Guard the post-upgrade tail: from here until newClient is
@@ -4008,7 +4051,12 @@ boolean isAborted() {
40084051

40094052
@Override
40104053
public WebSocketClient reconnect() {
4011-
return buildAndConnect(this);
4054+
return buildAndConnect(this, null);
4055+
}
4056+
4057+
@Override
4058+
public WebSocketClient reconnect(CursorWebSocketSendLoop.ConnectCancellation cancellation) {
4059+
return buildAndConnect(this, cancellation);
40124060
}
40134061
}
40144062
}

0 commit comments

Comments
 (0)