FWD » Core Development » Runtime Infrastructure

================================================================================================

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.

   +   * <p>

   +   * 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).