Address forceReconnectImpl reader/writer stop race

[scottf]

forceReconnectImpl tears the socket down with reader.stop(false) + an async port close + a 100 ms join, then proceeds to reconnectImpl() regardless of whether that join succeeded. Three facts combine into a race:

reader.stop(false) sets running=false but does NOT shutdownInput() — the reader thread stays blocked in dataPort.read(...); only the port close wakes it, and that runs on another thread.
The stopped future completes only when run() actually returns, so the .get(100ms) is really "wait up to 100 ms for the reader thread to die" — and on a busy executor / TLS socket it can miss.
tryToConnect re-joins the reader only if (reader.isRunning()) (V3: core/.../NatsConnection.java:516), which is now false — so it does NOT wait for a still-alive reader before calling reader.start(...) on the same instance.
A reader that outlives the 100 ms window can then, once the async close finally unblocks it: throw IOException, see running flipped back true by the new start(), call handleCommunicationIssue on the now-healthy connection (spurious full reconnect), and — via its finally { running.set(false) } — stomp the shared flag the freshly started reader loops on. Timing-dependent, worse under executor pressure and TLS. See the audit for the full trace.

[scottf]

@claude review

[synadia-claude-reviewer]

Claude finished @scottf's task in 2m 54s —— View job

---

Claude's Review

• Gather context and read the diff
• Read surrounding code for deeper context
• Analyze the fix for correctness
• Post full review

---

Overall Assessment

The fix is correct and well-reasoned. The root cause diagnosis in the PR description is accurate, and the three-part remedy (ordering, timeout, future capture) directly addresses each piece of the race.

---

What the Fix Gets Right

1. Critical ordering: stop(false) before port close

This is the crux of the fix. The reader's run() method in NatsConnectionReader.java:249 has this guard:

} catch (IOException io) {
    if (running.get()) {
        this.connection.handleCommunicationIssue(io);
    }
}

Previously, reader.stop(false) was called after the async port close was submitted. In a race, the port close could unblock the read while running was still true, causing handleCommunicationIssue to fire (spurious reconnect). By calling stop(false) first, running is false before the IOException arrives, so the catch block exits cleanly.

The finally { this.running.set(false); } stomp problem is also eliminated: if the new reader's start() sets running=true only after the join confirms the old reader has fully exited, the old reader's finally block can no longer stomp it.

2. Timeout extended from 100ms → connectionTimeout (default 2s)

The async port close is what actually unblocks the blocked dataPort.read(...). A fixed 100ms could expire before that close completes on a TLS socket or under executor pressure. The full connection timeout is the right budget here. In the common case the close lands in microseconds and the join returns immediately.

3. Future captured before async close

Future<Boolean> readerStopped = reader.isRunning() ? reader.stop(false) : null;

This captures the reader's current stopped future (the one belonging to the old reader run) before the async close and before reconnectImpl() could call reader.start() (which replaces stopped). This guarantees readerStopped.get(...) waits for the right thread, not a future new reader.

4. Guard against double-stop is correct

reader.isRunning() ? reader.stop(false) : null correctly skips the stop if the reader has already exited. The NatsConnectionReader.stop(false) is idempotent in the running→false direction, but the guard is clean and explicit.

---

Minor Observations

Independent timeouts for reader and writer

Both joins use the same timeoutNanos independently rather than sharing a shrinking budget like tryToConnect does. In the absolute worst case (both time out), this could add 2 × connectionTimeout (4s with defaults) before reconnectImpl() is called. In practice the close is nearly instantaneous so this is negligible, but it's worth noting as a behavior difference from tryToConnect's approach.

The tryToConnect backstop at line 591 is unchanged and still correct

if (reader.isRunning()) {
    this.reader.stop().get(timeoutNanos, TimeUnit.NANOSECONDS);
}

On the forceReconnectImpl path, isRunning() is false by the time tryToConnect is reached (the fix's stop(false) already cleared it), so this join is skipped — which is correct, because forceReconnectImpl already joined. For the normal reconnect() path this guard is still the primary join, which is unchanged.

Test coverage

No new tests are added, which is understandable — this race requires precise thread scheduling (busy executor + TLS) and is difficult to reproduce deterministically. The existing ReconnectTests.java and TLSConnectTests.java provide regression coverage for the happy path; a stress test exposing the race would require mock executor control or byteman-style injection.

Comments

The two large comment blocks are unusually long, but for a concurrency fix with this many interacting invariants, explaining why in place is valuable for future maintainers. The comments are accurate and precise.

---

Verdict

No blocking issues. The fix correctly addresses all three points in the race trace: ordering of stop vs. close, sufficient join timeout, and the tryToConnect guard bypass. Approve when ready.

[mtmk]

LGTM