================================================================================================ PROPOSAL - [MINOR] functional - WebClientProtocol.startKeepAlivePing The keep-alive timer reads pushWorker without holding lock, and the field is not volatile ================================================================================================ REVIEW FINDING (abridged) "the keep-alive TimerTask reads the pushWorker field from the websocket-keep-alive thread without holding lock, breaking the discipline every other access site observes - startPushWorker/ stopPushWorker are both documented 'must only be called by a caller that owns the instance lock', and sendTextMessage/sendBinaryMessage read it under lock. The field is not volatile, so this is a genuine data race: during onClose the lock holder writes pushWorker = null two statements before stopKeepAlivePing(), with a stopPushing() join of up to 5 s just ahead of it, and Timer.cancel() does not await an in-flight run. No behavioural difference is reachable today [...] so this is the 'documented lock discipline violated, no reachable consequence' case. Making the field volatile - as lastDataNanos correctly was - keeps the invariant honest." FILE src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ Two facts that only became relevant when r16675 added a second thread to the picture: WebClientProtocol.java:329 private PushMessagesWorker pushWorker; // plain field WebClientProtocol.java:1694 PushMessagesWorker worker = pushWorker; // read on the timer thread Every other access site holds the instance lock: sendTextMessage (:815-818) and sendBinaryMessage (:832-835) read it inside synchronized (lock), and startPushWorker/stopPushWorker (:1801, :1815) carry the javadoc "must only be called by a caller that owns the instance lock". The keep-alive tick is now the sole exception. The write/read pair is genuinely concurrent. In onClose (:757-759) the lock holder executes stopPushWorker(); // -> stopPushing() joins up to 5 s, then pushWorker = null stopWebWorker(); stopKeepAlivePing(); // -> Timer.cancel(), which does NOT await an in-flight run so a tick can be executing throughout, and Timer.cancel() does not wait for it. Without volatile there is no happens-before edge, so the timer thread may observe a stale non-null reference to a worker whose thread has already exited - or, in principle, an out-of-thin-air-free but arbitrarily stale value. Nothing reachable misbehaves today, and the finding says so: the timer thread is safely published by the Thread.start() inside new Timer(...) which follows startPushWorker in the same synchronized block; PushMessagesWorker.messages is final; a reconnect reuses the same worker instance; and every stale-read outcome on the teardown path is a no-op on a doomed tick. This is a latent-correctness fix, not a bug fix - but it is the kind that stops being latent the moment someone adds a second field to the tick. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Make the field volatile and say why ---------------------------------------- WebClientProtocol.java:328-329: - /** Dedicated worker to push the messages to the web-socket. */ - private PushMessagesWorker pushWorker; + /** + * Dedicated worker to push the messages to the web-socket. + *

+ * Written only under the instance lock (see {@link #startPushWorker} and + * {@link #stopPushWorker}). Volatile because the keep-alive timer thread reads it outside that + * lock: {@code Timer.cancel()} does not await an in-flight run, so a tick can still be executing + * while {@link #onClose} nulls this field. + */ + private volatile PushMessagesWorker pushWorker; (The exact existing comment text at :328 should be preserved as the first line; adjust if it differs.) This is the review's own recommendation and matches the precedent set by lastDataNanos in the same revision. 2.2 Put the send back under the lock ------------------------------------- The tick has two interactions with the worker, and they want different treatment: (a) The *send*. Today it goes through sendBinaryMessage(MSG_SERVER_PING), which already synchronizes on lock, so it is correct as-is. proposal-issue-major-performance-1 replaces it with a sendKeepAlivePing() helper that also synchronizes on lock - deliberately, so the priority push does not become a new discipline violation: private void sendKeepAlivePing() { synchronized (lock) { if (pushWorker != null) { pushWorker.pushPriorityMessage(ByteBuffer.wrap(new byte[] { MSG_SERVER_PING })); } } } (b) The *observation* (getTransmittedCount / getQueueSize). This is a snapshot read for a heuristic; taking the instance lock for it on every tick is undesirable. onClose holds the lock across a stopPushing() join of up to 5 s, so a lock-taking observation would block the timer thread for that whole window - and the timer thread is the one thing that must never wedge. Leave it lock- free, which volatile now makes well-defined, and document the exception at the read site: // read without the instance lock on purpose: onClose holds it across a stopPushing() join of // up to WAIT_TO_DIE, and the keep-alive tick must never block on that. The field is volatile, // and a stale-but-published worker only ever costs this tick a heuristic, never correctness. PushMessagesWorker worker = pushWorker; 2.3 Also close the stale-tick window properly ---------------------------------------------- Optional, and worth doing while in the area. Because Timer.cancel() does not await an in-flight run, a tick that has already passed the session.isOpen() check can still call session.close() *after* onClose has finished - on the session it was created for, which is harmless, but the same is not obviously true once the tick does more work. Two cheap belts: (a) The tick already captures `session` as its own final parameter and checks isOpen(), which is the right pattern; keep it and do not replace it with a read of this.session. (b) Give the task a `cancelled` flag set by stopKeepAlivePing() before cancel(), and check it first in run(). This makes "a cancelled timer performs no observable action" true by construction rather than by case analysis: private void stopKeepAlivePing() { if (pingTimer != null) { pingTimer.cancel(); pingTimer = null; LOG.info("Keep-alive ping stopped!"); } } becomes, with the task held in a field alongside the timer, `keepAliveTask.cancel()` (TimerTask. cancel() also does not await, so the flag is still what does the work) plus an early `if (isCancelled()) return;` in run(). Judgement call: this is defensive rather than fixing anything observable, and it adds a field. Recommend 2.1 + 2.2 unconditionally, and 2.3(b) only if the tick grows further. ------------------------------------------------------------------------------------------------ 3. WHY NOT SYNCHRONIZE THE WHOLE TICK ------------------------------------------------------------------------------------------------ Wrapping run() in synchronized (lock) would restore the documented discipline uniformly, but it makes the keep-alive thread contend with - and, during teardown, block for up to PushMessagesWorker.WAIT_TO_DIE (5 s) behind - the lock held across stopPushing(). The heartbeat is the last line of defence for reclaiming a wedged session; it must not be blockable by the very teardown path it may need to trigger. Volatile plus a lock-scoped send is the right balance, and documenting the one deliberate exception is what keeps the invariant honest. ------------------------------------------------------------------------------------------------ 4. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-performance-1 introduces sendKeepAlivePing(), the lock-scoped send in 2.2(a) proposal-issue-major-functional-1 adds the getTransmittedCount() read covered by 2.2(b) ------------------------------------------------------------------------------------------------ 5. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: none functionally. Volatile on a reference read once per tick and a handful of times per message send is not measurable; pushMessage/pushPriorityMessage already take a monitor on `messages`. Verification 1. Compile and run the existing Web GUI/CHUI smoke path; there is no unit test around this class. 2. Repeated connect/disconnect/reconnect cycling (30x) with the keep-alive interval forced to 1000 ms (client/web/pingPongInterval=1000) to maximise the overlap between a tick and onClose. Expect no NPE, no "Keep-alive ping could not be sent!" warnings, and a clean "Keep-alive ping stopped!" per close. 3. Kill the browser mid-burst so stopPushWorker's join actually takes time, and confirm the timer thread is not blocked (thread dump during the window, or timestamps on the tick log lines).