Skip to content

Commit 0e5125f

Browse files
committed
fix(qwp): finite default connect deadline for background drainers; CAS-guard WebSocketClient.close() (C4 residuals)
With connect_timeout unset (0), a drainer parked in an untimed native connect (nf.connectAddrInfo, OS-bounded: 60-130s of SYN retries on Linux) is the normal state during an outage. The pool's shutdownNow path (~3s into sender.close()) then reliably lands on the failed-stop teardown protocol: the WebSocket client and microbatch buffers are deliberately leaked and the SF slot lock is held until the OS deadline resolves the stuck connect. Background connect walks now default to a finite 15s deadline; an explicit connect_timeout is honoured verbatim on both paths and the foreground's unset default stays untimed (no behavior change). WebSocketClient.close() frees native memory but was gated by a volatile check-then-act -- two concurrent closers could both enter and double-run disconnect()/Unsafe.free. Now a CAS on an AtomicBoolean: exactly one closer runs teardown. No concurrent-closer path is reachable at HEAD (the failed-stop protocol severed them), so this is defense in depth that makes the 'duplicate closes are safe' comments in CursorWebSocketSendLoop true unconditionally rather than by-current-call-graph. Tests: BackgroundConnectTimeoutDefaultTest covers the resolver matrix; WebSocketClientTest.testConcurrentCloseRunsTeardownExactlyOnce races 4 closers x 200 iterations under assertMemoryLeak, where a double-free surfaces as a native memory-counter mismatch.
1 parent e496e62 commit 0e5125f

4 files changed

Lines changed: 165 additions & 10 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/http/client/WebSocketClient.java

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import java.security.MessageDigest;
4848
import java.security.NoSuchAlgorithmException;
4949
import java.util.Base64;
50+
import java.util.concurrent.atomic.AtomicBoolean;
5051

5152
import static java.util.concurrent.TimeUnit.NANOSECONDS;
5253

@@ -99,8 +100,11 @@ public abstract class WebSocketClient implements QuietCloseable {
99100
private final int maxRecvBufSize;
100101
private final SecureRnd rnd;
101102
private final WebSocketSendBuffer sendBuffer;
102-
// volatile: written by user thread in close(), read by I/O thread in checkConnected()/sendFrame()/receiveFrame()
103-
private volatile boolean closed;
103+
// Written by whichever closer wins the CAS in close(); read by the I/O
104+
// thread in checkConnected()/sendFrame()/receiveFrame(). An AtomicBoolean
105+
// (not a bare volatile check-then-act) so concurrent closers cannot both
106+
// enter close() and double-run disconnect()/Unsafe.free.
107+
private final AtomicBoolean closed = new AtomicBoolean();
104108
// Upper bound (ms) on the TCP connect. <= 0 disables the application-level
105109
// timeout and falls back to the OS connect timeout. Seeded from the
106110
// configuration; the QWP sender may override it via setConnectTimeout().
@@ -197,7 +201,7 @@ public WebSocketClient(HttpClientConfiguration configuration, SocketFactory sock
197201
this.frameParser = new WebSocketFrameParser();
198202
this.rnd = new SecureRnd();
199203
this.upgraded = false;
200-
this.closed = false;
204+
this.closed.set(false);
201205
} catch (Throwable t) {
202206
if (recvBufPtr != 0) {
203207
Unsafe.free(recvBufPtr, recvBufSize, MemoryTag.NATIVE_DEFAULT);
@@ -212,8 +216,12 @@ public WebSocketClient(HttpClientConfiguration configuration, SocketFactory sock
212216

213217
@Override
214218
public void close() {
215-
if (!closed) {
216-
closed = true;
219+
// CAS gate: exactly one closer runs the teardown below. Closers can be
220+
// the owner thread, the I/O thread's exit path, or a stale duplicate
221+
// reference (see CursorWebSocketSendLoop) -- a bare volatile
222+
// check-then-act here would let two concurrent closers both enter and
223+
// double-run disconnect()/Unsafe.free (native double-free).
224+
if (closed.compareAndSet(false, true)) {
217225

218226
// Try to send close frame
219227
if (upgraded && !socket.isClosed()) {
@@ -247,7 +255,7 @@ public void close() {
247255
* @param port the server port
248256
*/
249257
public void connect(CharSequence host, int port) {
250-
if (closed) {
258+
if (closed.get()) {
251259
throw new HttpClientException("WebSocket client is closed");
252260
}
253261

@@ -380,7 +388,7 @@ public int getUpgradeStatusCode() {
380388
* Returns whether the WebSocket is connected and upgraded.
381389
*/
382390
public boolean isConnected() {
383-
return upgraded && !closed && !socket.isClosed();
391+
return upgraded && !closed.get() && !socket.isClosed();
384392
}
385393

386394
/**
@@ -585,7 +593,7 @@ public boolean tryReceiveFrame(WebSocketFrameHandler handler) {
585593
* @param authorizationHeader the Authorization header value (e.g., "Basic ..."), or null
586594
*/
587595
public void upgrade(CharSequence path, int timeout, CharSequence authorizationHeader) {
588-
if (closed) {
596+
if (closed.get()) {
589597
throw new HttpClientException("WebSocket client is closed");
590598
}
591599
if (socket.isClosed()) {
@@ -892,7 +900,7 @@ private void appendToFragmentBuffer(long payloadPtr, int payloadLen) {
892900
}
893901

894902
private void checkConnected() {
895-
if (closed) {
903+
if (closed.get()) {
896904
throw new HttpClientException("WebSocket client is closed");
897905
}
898906
if (!upgraded) {

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,9 @@ public class QwpWebSocketSender implements Sender {
127127
public static final int DEFAULT_AUTO_FLUSH_BYTES = 8 * 1024 * 1024;
128128
public static final long DEFAULT_AUTO_FLUSH_INTERVAL_NANOS = 100_000_000L; // 100ms
129129
public static final int DEFAULT_AUTO_FLUSH_ROWS = 1_000;
130+
// Finite fallback (ms) for BACKGROUND (drainer) TCP connects when the
131+
// user left connect_timeout unset. See effectiveConnectTimeoutMs.
132+
public static final int DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS = 15_000;
130133
private static final int DEFAULT_BUFFER_SIZE = 8192;
131134
private static final int DEFAULT_MICROBATCH_BUFFER_SIZE = 1024 * 1024; // 1MB
132135
private static final Logger LOG = LoggerFactory.getLogger(QwpWebSocketSender.class);
@@ -2445,6 +2448,25 @@ private void atNanos(long timestampNanos) {
24452448
sendRow();
24462449
}
24472450

2451+
/**
2452+
* Resolves the connect timeout for one {@code buildAndConnect} walk.
2453+
* Foreground connects honour the configured value verbatim: 0 (the
2454+
* default) keeps the historical untimed native connect, bounded only by
2455+
* the OS (SYN retries, 60-130s on Linux). Background (drainer) connects
2456+
* get a finite fallback instead: during an outage a drainer is routinely
2457+
* parked inside a blocking native connect that neither unpark nor
2458+
* interrupt cancels, so the drainer pool's shutdownNow path (~3s into
2459+
* sender.close()) reliably lands on the failed-stop protocol -- the
2460+
* WebSocket client and microbatch buffers are deliberately leaked and
2461+
* the slot lock is held until the OS deadline resolves the connect. A
2462+
* finite background deadline bounds that window to seconds without
2463+
* changing foreground semantics. Exposed for unit tests.
2464+
*/
2465+
@TestOnly
2466+
public static int effectiveConnectTimeoutMs(boolean background, int configuredMs) {
2467+
return background && configuredMs <= 0 ? DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS : configuredMs;
2468+
}
2469+
24482470
private synchronized WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
24492471
// Background (drainer) factories share this connect walk -- endpoint
24502472
// list, hostTracker health and round state -- but must stay INVISIBLE
@@ -2517,7 +2539,7 @@ private synchronized WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
25172539
newClient.setQwpMaxVersion(QwpConstants.VERSION);
25182540
newClient.setQwpClientId(QwpConstants.CLIENT_ID);
25192541
newClient.setQwpRequestDurableAck(requestDurableAck);
2520-
newClient.setConnectTimeout(connectTimeoutMs);
2542+
newClient.setConnectTimeout(effectiveConnectTimeoutMs(background, connectTimeoutMs));
25212543
newClient.connect(ep.host, ep.port);
25222544
int upgradeTimeoutMs = (int) Math.min(authTimeoutMs, Integer.MAX_VALUE);
25232545
newClient.upgrade(WRITE_PATH, upgradeTimeoutMs, authorizationHeader);

core/src/test/java/io/questdb/client/test/cutlass/http/client/WebSocketClientTest.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,55 @@
3737

3838
import java.lang.reflect.Field;
3939
import java.lang.reflect.Method;
40+
import java.util.concurrent.CyclicBarrier;
41+
import java.util.concurrent.atomic.AtomicReference;
4042

4143
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4244

4345
public class WebSocketClientTest {
4446

47+
/**
48+
* close() frees native memory (recv/fragment buffers, send buffers), so
49+
* its guard must be a CAS, not a volatile check-then-act: two concurrent
50+
* closers passing the flag check together would both run
51+
* disconnect()/Unsafe.free -- a native double-free. Closers can race in
52+
* practice: the owner thread's teardown vs the I/O thread's exit path vs
53+
* stale duplicate references (see CursorWebSocketSendLoop). The memory
54+
* counters checked by assertMemoryLeak flag a double-free as a counter
55+
* mismatch.
56+
*/
57+
@Test
58+
public void testConcurrentCloseRunsTeardownExactlyOnce() throws Exception {
59+
assertMemoryLeak(() -> {
60+
final int threads = 4;
61+
final int iterations = 200;
62+
for (int i = 0; i < iterations; i++) {
63+
StubWebSocketClient client = new StubWebSocketClient();
64+
CyclicBarrier barrier = new CyclicBarrier(threads);
65+
AtomicReference<Throwable> failure = new AtomicReference<>();
66+
Thread[] closers = new Thread[threads];
67+
for (int t = 0; t < threads; t++) {
68+
closers[t] = new Thread(() -> {
69+
try {
70+
barrier.await();
71+
client.close();
72+
} catch (Throwable e) {
73+
failure.compareAndSet(null, e);
74+
}
75+
});
76+
closers[t].start();
77+
}
78+
for (Thread closer : closers) {
79+
closer.join();
80+
}
81+
Throwable t = failure.get();
82+
if (t != null) {
83+
throw new AssertionError("concurrent close failed on iteration " + i, t);
84+
}
85+
}
86+
});
87+
}
88+
4589
@Test
4690
public void testExtractMaxBatchSizeAbsentHeaderReturnsZero() throws Exception {
4791
String response = "HTTP/1.1 101 Switching Protocols\r\n"
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/*+*****************************************************************************
2+
* ___ _ ____ ____
3+
* / _ \ _ _ ___ ___| |_| _ \| __ )
4+
* | | | | | | |/ _ \/ __| __| | | | _ \
5+
* | |_| | |_| | __/\__ \ |_| |_| | |_) |
6+
* \__\_\\__,_|\___||___/\__|____/|____/
7+
*
8+
* Copyright (c) 2014-2019 Appsicle
9+
* Copyright (c) 2019-2026 QuestDB
10+
*
11+
* Licensed under the Apache License, Version 2.0 (the "License");
12+
* you may not use this file except in compliance with the License.
13+
* You may obtain a copy of the License at
14+
*
15+
* http://www.apache.org/licenses/LICENSE-2.0
16+
*
17+
* Unless required by applicable law or agreed to in writing, software
18+
* distributed under the License is distributed on an "AS IS" BASIS,
19+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
20+
* See the License for the specific language governing permissions and
21+
* limitations under the License.
22+
*
23+
******************************************************************************/
24+
25+
package io.questdb.client.test.cutlass.qwp.client;
26+
27+
import io.questdb.client.cutlass.qwp.client.QwpWebSocketSender;
28+
import org.junit.Assert;
29+
import org.junit.Test;
30+
31+
import static io.questdb.client.cutlass.qwp.client.QwpWebSocketSender.DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS;
32+
import static io.questdb.client.cutlass.qwp.client.QwpWebSocketSender.effectiveConnectTimeoutMs;
33+
34+
/**
35+
* Background (drainer) connect walks must never inherit the untimed native
36+
* connect that connect_timeout=0 (the default) means for the foreground.
37+
* <p>
38+
* During an outage a drainer is routinely parked inside a blocking native
39+
* connect ({@code nf.connectAddrInfo}) that neither unpark nor interrupt
40+
* cancels. The drainer pool's close sequence (2.5s graceful drain +
41+
* requestStop + 500ms + shutdownNow) then reliably lands on the failed-stop
42+
* teardown protocol: the WebSocket client and microbatch buffers are
43+
* deliberately leaked and the SF slot lock is held until the OS connect
44+
* deadline (SYN retries, 60-130s on Linux) resolves the stuck call. A finite
45+
* background default bounds that window to seconds. Foreground semantics are
46+
* intentionally untouched: an explicit user value is honoured verbatim on
47+
* both paths, and the foreground's unset default stays untimed.
48+
*/
49+
public class BackgroundConnectTimeoutDefaultTest {
50+
51+
@Test
52+
public void testBackgroundExplicitValueHonoured() {
53+
Assert.assertEquals(500, effectiveConnectTimeoutMs(true, 500));
54+
Assert.assertEquals(60_000, effectiveConnectTimeoutMs(true, 60_000));
55+
}
56+
57+
@Test
58+
public void testBackgroundUnsetGetsFiniteDefault() {
59+
Assert.assertEquals(DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS, effectiveConnectTimeoutMs(true, 0));
60+
// Defensive: builder validation rejects negatives, but the resolver
61+
// must not turn a bad value back into an untimed background connect.
62+
Assert.assertEquals(DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS, effectiveConnectTimeoutMs(true, -1));
63+
}
64+
65+
@Test
66+
public void testDefaultIsFinite() {
67+
Assert.assertTrue(DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS > 0);
68+
}
69+
70+
@Test
71+
public void testForegroundExplicitValueHonoured() {
72+
Assert.assertEquals(500, effectiveConnectTimeoutMs(false, 500));
73+
}
74+
75+
@Test
76+
public void testForegroundUnsetStaysUntimed() {
77+
// 0 => WebSocketClient falls back to nf.connectAddrInfo (OS-bounded).
78+
// Historical foreground behaviour, deliberately preserved.
79+
Assert.assertEquals(0, effectiveConnectTimeoutMs(false, 0));
80+
}
81+
}

0 commit comments

Comments
 (0)