Skip to content

Commit f6dfb32

Browse files
committed
test(qwp): red-first -- interrupted close must not leak a client installed by an in-flight reconnect (C5)
CursorWebSocketSendLoop.close() swallows the InterruptedException from shutdownLatch.await(), re-interrupts, and returns while the I/O thread is still alive inside a blocking connect (connect_timeout defaults to 0 = OS timeout; neither unpark nor interrupt cancels connect(2)). Under Invariant B drainers no longer exit on a wall-clock budget during outages, so BackgroundDrainerPool.close()'s shutdownNow() escalation makes this path routine: the interrupt lands in the drainer's loop.close() latch await, the teardown falls through, and a subsequently succeeding reconnect() installs a live client into the abandoned loop that nothing ever closes -- its native socket, fds and buffers leak for the life of the process. The test pins the fix-agnostic ownership contract: every WebSocketClient the loop obtains (constructor or factory) must be closed by the time the loop is quiescent. Deterministic: the interrupt is injected by pre-setting the closer thread's flag (CountDownLatch.await() checks Thread.interrupted() before parking) and the stuck connect is a latch-gated ReconnectFactory (unpark- immune, close() never interrupts the I/O thread). Verified green under a scratch re-await fix; red on current HEAD. The SEGV half of C5 (engine unmapped under the live I/O thread) is intentionally not covered here -- a crash is not assertable in-process and its teardown-overlap proxy invariant pins a protocol choice; see review notes.
1 parent 70c706a commit f6dfb32

1 file changed

Lines changed: 208 additions & 0 deletions

File tree

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
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.sf.cursor;
26+
27+
import io.questdb.client.DefaultHttpClientConfiguration;
28+
import io.questdb.client.cutlass.http.client.WebSocketClient;
29+
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
30+
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
31+
import io.questdb.client.network.PlainSocketFactory;
32+
import io.questdb.client.std.Compat;
33+
import io.questdb.client.test.tools.TestUtils;
34+
import org.junit.Assert;
35+
import org.junit.Test;
36+
37+
import java.util.concurrent.CountDownLatch;
38+
import java.util.concurrent.TimeUnit;
39+
import java.util.concurrent.atomic.AtomicInteger;
40+
import java.util.concurrent.atomic.AtomicReference;
41+
42+
/**
43+
* Red test for finding C5 — interrupted drainer teardown abandons a client
44+
* installed by an in-flight reconnect.
45+
* <p>
46+
* Production sequence being modeled: during a server outage an orphan
47+
* drainer's {@link CursorWebSocketSendLoop} I/O thread sits inside a
48+
* blocking native connect ({@code connect_timeout} defaults to 0 = OS
49+
* timeout, tens of seconds; neither {@code unpark} nor interrupt cancels
50+
* {@code connect(2)}). Under Invariant B the drainer no longer exits on a
51+
* wall-clock budget, so {@code BackgroundDrainerPool.close()} routinely
52+
* escalates: 2500&nbsp;ms graceful drain &rarr; {@code requestStop()} &rarr;
53+
* 500&nbsp;ms grace &rarr; {@code shutdownNow()}. The {@code shutdownNow()}
54+
* interrupt lands in {@code loop.close()}'s {@code shutdownLatch.await()};
55+
* pre-fix, {@code close()} swallows the {@link InterruptedException},
56+
* re-interrupts, and returns while the I/O thread is still alive. When the
57+
* in-flight {@code reconnect()} subsequently succeeds, {@code swapClient}
58+
* installs the live client into the abandoned loop — and no code path ever
59+
* closes it: {@code loop.close()} already ran (its {@code client} read saw
60+
* null), and {@code ioLoop}'s exit path only counts down the latch. The
61+
* client's native socket, fds and buffers leak for the life of the process.
62+
* <p>
63+
* The test pins the fix-agnostic ownership contract, not a fix strategy:
64+
* <b>every {@code WebSocketClient} the loop obtains — via constructor or
65+
* factory — must be closed by the time the loop is quiescent (I/O thread
66+
* exited, {@code close()} completed or failed loudly).</b> Any of the
67+
* candidate fixes satisfies it: (a) re-awaiting the shutdown latch in a
68+
* loop (close() then picks up the swapped client), (b) closing the current
69+
* client in {@code ioLoop}'s exit path, or (c) {@code connectLoop}
70+
* discarding-and-closing a factory client obtained after {@code running}
71+
* went false. A guard-only fix that merely skips engine teardown (the
72+
* SEGV half of C5) correctly leaves this test red — the leak is a distinct
73+
* defect.
74+
* <p>
75+
* Determinism notes: no sleeps or timing races. The interrupt is injected
76+
* by pre-setting the closer thread's interrupt flag —
77+
* {@code CountDownLatch.await()} checks {@code Thread.interrupted()} before
78+
* parking, so the swallow path is entered on the first call. The "stuck
79+
* native connect" is a factory blocked on a test latch, which is faithful:
80+
* a latch await re-parks after {@code close()}'s spurious {@code unpark},
81+
* and {@code close()} never interrupts the I/O thread.
82+
*/
83+
public class CursorWebSocketSendLoopInterruptedCloseLeakTest {
84+
85+
@Test
86+
public void testC5_interruptedCloseMustNotLeakClientInstalledByInFlightReconnect() throws Exception {
87+
TestUtils.assertMemoryLeak(() -> {
88+
final CountDownLatch enteredReconnect = new CountDownLatch(1);
89+
final CountDownLatch releaseConnect = new CountDownLatch(1);
90+
final AtomicReference<Thread> ioThreadRef = new AtomicReference<>();
91+
final TrackingStubWebSocketClient liveClient = new TrackingStubWebSocketClient();
92+
93+
// Stand-in for a blocking native connect(2): entered by the loop's
94+
// I/O thread, immune to unpark, never interrupted by loop.close().
95+
// Returns a live client once released — the "reconnect succeeds
96+
// mid-teardown" arm of C5.
97+
final CursorWebSocketSendLoop.ReconnectFactory stuckConnect = () -> {
98+
ioThreadRef.set(Thread.currentThread());
99+
enteredReconnect.countDown();
100+
releaseConnect.await();
101+
return liveClient;
102+
};
103+
104+
final CursorSendEngine engine = new CursorSendEngine(null, 64 * 1024);
105+
try {
106+
CursorWebSocketSendLoop loop = new CursorWebSocketSendLoop(
107+
null /* async-initial-connect: the I/O thread drives the connect */,
108+
engine, 0L, 1_000L,
109+
stuckConnect,
110+
5_000L, 100L, 5_000L, false);
111+
loop.start();
112+
Assert.assertTrue("I/O thread never reached the reconnect factory",
113+
enteredReconnect.await(5, TimeUnit.SECONDS));
114+
115+
// Drainer-thread stand-in: BackgroundDrainer.run()'s finally calls
116+
// loop.close() and shutdownNow()'s interrupt lands in the latch
117+
// await. Pre-setting the flag makes that deterministic.
118+
final AtomicReference<Throwable> closeFailure = new AtomicReference<>();
119+
Thread closer = new Thread(() -> {
120+
Thread.currentThread().interrupt();
121+
try {
122+
loop.close();
123+
} catch (Throwable t) {
124+
// A close() that THROWS to signal the failed stop is a
125+
// valid fix shape (QwpWebSocketSender.close()'s
126+
// ioThreadStopped guard consumes exactly that signal).
127+
// The ownership assertion below is what must hold.
128+
closeFailure.set(t);
129+
}
130+
}, "drainer-close-stand-in");
131+
closer.setDaemon(true);
132+
closer.start();
133+
134+
// close()'s first action is running=false. Once observable, the
135+
// teardown is underway and the "connect" may complete. Under a
136+
// re-await fix the closer is still blocked inside close() here,
137+
// so the gate must open before joining it (fix-agnostic order).
138+
long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(5);
139+
while (loop.isRunning()) {
140+
Assert.assertTrue("close() never started", System.nanoTime() < deadlineNanos);
141+
Compat.onSpinWait();
142+
}
143+
releaseConnect.countDown();
144+
145+
closer.join(5_000L);
146+
Assert.assertFalse("closer thread did not finish", closer.isAlive());
147+
Thread ioThread = ioThreadRef.get();
148+
Assert.assertNotNull(ioThread);
149+
ioThread.join(5_000L);
150+
Assert.assertFalse("I/O thread did not exit after the connect returned",
151+
ioThread.isAlive());
152+
153+
// Loop is quiescent. Capture the verdict BEFORE any cleanup so
154+
// the test's own close calls cannot mask the leak.
155+
boolean closedByLoop = liveClient.closeCount() > 0;
156+
157+
Assert.assertTrue(
158+
"C5: the WebSocketClient handed to the loop by an in-flight "
159+
+ "reconnect() was never closed. loop.close() swallowed the "
160+
+ "InterruptedException from shutdownLatch.await() and returned "
161+
+ "while the I/O thread was still inside the blocking connect; "
162+
+ "swapClient then installed the live client into the abandoned "
163+
+ "loop where nothing closes it — its native socket and fds leak "
164+
+ "past drainer teardown. Every client the loop obtains "
165+
+ "(constructor or factory) must be closed by the time the loop "
166+
+ "is quiescent.",
167+
closedByLoop);
168+
} finally {
169+
liveClient.close();
170+
engine.close();
171+
}
172+
});
173+
}
174+
175+
/**
176+
* Minimal concrete {@link WebSocketClient} — never performs I/O; counts
177+
* {@code close()} calls so the test can assert ownership at quiescence.
178+
* Close remains idempotent via the superclass, matching the production
179+
* contract owners rely on.
180+
*/
181+
private static final class TrackingStubWebSocketClient extends WebSocketClient {
182+
private final AtomicInteger closeCount = new AtomicInteger();
183+
184+
TrackingStubWebSocketClient() {
185+
super(DefaultHttpClientConfiguration.INSTANCE, PlainSocketFactory.INSTANCE);
186+
}
187+
188+
@Override
189+
public void close() {
190+
closeCount.incrementAndGet();
191+
super.close();
192+
}
193+
194+
int closeCount() {
195+
return closeCount.get();
196+
}
197+
198+
@Override
199+
protected void ioWait(int timeout, int op) {
200+
throw new UnsupportedOperationException("stub: no socket");
201+
}
202+
203+
@Override
204+
protected void setupIoWait() {
205+
// no-op
206+
}
207+
}
208+
}

0 commit comments

Comments
 (0)