FWD » Core Development » Runtime Infrastructure

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

PROPOSAL - [MINOR] functional - PushMessagesWorker.getQueueSize

Dead method whose javadoc documents a guard that was never built

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

REVIEW FINDING (abridged)

   "Newly added by this change with no caller anywhere in the tree [...] yet its javadoc asserts a

   use that does not exist: 'Used by the keep-alive timer to tell its own silence apart from a

   backlog' [...] The claim is not merely unimplemented but unimplementable as written."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java

------------------------------------------------------------------------------------------------

1. ROOT CAUSE

------------------------------------------------------------------------------------------------

The method (PushMessagesWorker.java:398-409) is added by this revision and has no caller anywhere -

grep across src/, test/ and tools/ finds only the declaration. Its javadoc nevertheless says:

   * Used by the keep-alive timer to tell its own silence apart from a backlog: while output is

   * queued the peer may simply not have been asked anything yet, so its silence proves nothing.

WebClientProtocol.startKeepAlivePing's TimerTask reads pushWorker only to call pushMessageFirst and

decides peer death purely from dataQuietMs >= dataIdleMs - tickMs. There is no backlog term.

The claim is not just unimplemented, it is unimplementable in this form, for two independent

reasons:

   (a) The premise is already false. pushMessageFirst (:280) enqueues the heartbeat with offerFirst

       and bypasses awaitCapacity entirely, so a deep queue cannot delay the ping behind it. The

       backlog the javadoc worries about does not in fact keep the peer from being asked.

   (b) The measurement would not see the case that matters. sendMessage polls the message off the

       deque *before* the blocking write, so during the only scenario that can genuinely starve the

       heartbeat - a stalled send, where the peer has stopped draining its socket - getQueueSize()

       reads 0. The proposed guard would be blind precisely when it was needed.

Separately, ConcurrentLinkedDeque.size() is a documented O(n) traversal, so it would be the wrong

primitive for a per-tick check even if a depth check were wanted.

Leaving this in place is a maintenance hazard rather than a runtime one: the next person to read

startKeepAlivePing will believe a backlog guard exists somewhere and reason about the heartbeat on

that basis.

------------------------------------------------------------------------------------------------

2. PROPOSED FIX

------------------------------------------------------------------------------------------------

2.1 Delete the method

-----------------------

   -  /**

   -   * The number of messages still waiting to be transmitted.

   -   * <p>

   -   * Used by the keep-alive timer to tell its own silence apart from a backlog: while output is

   -   * queued the peer may simply not have been asked anything yet, so its silence proves nothing.

   -   *

   -   * @return   The current depth of the outbound queue.

   -   */

   -  public int getQueueSize()

   -  {

   -     return messages.size();

   -  }

Nothing else changes; there are no callers to update. The class already exposes queue pressure where

it is genuinely needed, through awaitCapacity/queuedBytes, which is byte-based and O(1).

2.2 Record why, where the next reader will look

-------------------------------------------------

The reasoning above is worth keeping, but it belongs next to the decision it explains, not on a

deleted method. Add it to startKeepAlivePing's javadoc in WebClientProtocol, which already documents

the deadline design:

   +   * <p>

   +   * The deadline deliberately does not consider the outbound queue depth.  A backlog cannot delay

   +   * the heartbeat - {@link PushMessagesWorker#pushMessageFirst} enqueues it at the head and skips

   +   * the backpressure gate - and the one condition that can starve it, a send stalled on a peer

   +   * that has stopped reading, leaves the queue reading empty because the message has already been

   +   * polled off it.  A peer that has not accepted a byte for a whole deadline is correctly closed.

2.3 If a backlog guard is actually wanted

-------------------------------------------

It should not be built on queue depth. The measurable signal is *completed writes*: a monotonic

counter incremented in sendMessage after a successful send would distinguish "the socket is still

draining, the page is just behind" from "nothing is moving at all", and unlike queue depth it is not

blind to the in-flight message. That is a design change to the deadline, not a javadoc fix, and

should be raised as its own task with a stated failure case; it is out of scope here.

------------------------------------------------------------------------------------------------

3. RELATIONSHIP TO OTHER PROPOSALS

------------------------------------------------------------------------------------------------

None. This is self-contained.

Note for the history block: the method was added under entry 010 in this same revision, so removing

it needs no new history entry if the change is folded into the same commit; if it lands separately,

add one rather than editing 010.

------------------------------------------------------------------------------------------------

4. RISK AND VERIFICATION

------------------------------------------------------------------------------------------------

Risk: none. The method is unreferenced.

Verification

   1. grep -rn "getQueueSize" src/ test/ tools/ returns nothing after the change.

   2. ./gradlew compile succeeds.

   3. Confirm no rules/ TRPL or reflection-based lookup names the method (grep the whole tree, not

      just the Java source) - it is a public method on a public class, so this is worth one check

      even though no such usage is plausible.