================================================================================================ PROPOSAL - [MINOR] functional - WebClientProtocol.startKeepAlivePing MIN_PING_INTERVAL floor is applied after the idle-timeout clamp, so it overrides it ================================================================================================ REVIEW FINDING (abridged) "interval = Math.max(interval, MIN_PING_INTERVAL) is applied *after* the idle-timeout clamp, so for any idle timeout below PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL (3000 ms) the floor silently overrides it and the invariant the javadoc asserts ('where an idle timeout is in force it is also held to one PING_INTERVAL_DIVISORth of it') does not hold: idleMs=2000 gives 1000, i.e. half the idle timeout rather than a third [...] and idleMs=1000 gives interval == idleMs, so since the ping is only queued to PushMessagesWorker rather than written on the timer thread it cannot reach the wire before Jetty's deadline - the session is reaped on the first quiet second, producing a reconnect loop that the correctly-ordered clamp (333 ms) would have avoided. [...] Clamp in the other order, i.e. floor the configured cadence and let the idle-derived bound win." FILE src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ WebClientProtocol.java:1658-1661: long interval = (idleMs > 0) ? Math.min(pingPongInterval, idleMs / PING_INTERVAL_DIVISOR) : pingPongInterval; interval = Math.max(interval, MIN_PING_INTERVAL); Two clamps with opposite intent are applied in the wrong order. The floor exists to stop an absurdly small *configured cadence* from flooding the socket; the ceiling exists to guarantee a *margin* below the idle timeout. Applying the floor last lets it undo the ceiling: idleMs ceiling (idleMs/3) result effective divisor ------- ------------------- ------- ----------------- 3000 1000 1000 3 (boundary, correct) 2000 666 1000 2 (margin halved) 1000 333 1000 1 (no margin at all) At idleMs == 1000 the ping is scheduled exactly at the idle deadline, and since it is only *queued* to PushMessagesWorker rather than written on the timer thread, it cannot reach the wire in time - Jetty reaps the session on the first quiet second and the client enters a reconnect loop. The correctly ordered clamp yields 333 ms and avoids it. Reachability: client/web/socketTimeout and client/web/maxIdleTime are plain Integers with no lower bound (ConfigItem.java:374-375, :363-364) and dir_schema.xml has no client/web object class, so a value of 1000 or 2000 is accepted verbatim. Low values are plausible in test and diagnostic setups. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ Floor the configured cadence first, then let the idle-derived bound win: - // The configured cadence is the baseline. When an idle timeout is in force the interval is also - // held below it, since a ping landing at or after that deadline would be useless. When there is - // no idle timeout the clamp is simply skipped - the heartbeat is not made conditional on one, - // because without it Jetty never reaps the socket either and this check becomes the only dead - // peer detection there is. - long interval = (idleMs > 0) ? Math.min(pingPongInterval, idleMs / PING_INTERVAL_DIVISOR) - : pingPongInterval; - - interval = Math.max(interval, MIN_PING_INTERVAL); + // The configured cadence is the baseline, floored so that a tiny configured value cannot flood + // the socket. The idle-derived bound is applied last and wins: it is a correctness bound, not a + // preference - a ping landing at or after the idle deadline can never reach the wire in time, + // since it is queued to the push worker rather than written on this thread. Applying the floor + // last would let it override that bound for any idle timeout below + // PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL. When there is no idle timeout the bound is simply + // skipped - the heartbeat is not made conditional on one, because without it Jetty never reaps + // the socket either and this check becomes the only dead peer detection there is. + long interval = Math.max(pingPongInterval, MIN_PING_INTERVAL); + + if (idleMs > 0) + { + interval = Math.min(interval, Math.max(1L, idleMs / PING_INTERVAL_DIVISOR)); + } The Math.max(1L, ...) guards the degenerate idleMs < PING_INTERVAL_DIVISOR case: Timer.schedule throws IllegalArgumentException on a non-positive period, and idleMs == 1 or 2 would otherwise produce 0. The resulting interval is nonsensical at that point, but the session's own idle timeout is nonsensical too, and a 1 ms ping is preferable to an uncaught exception on the connect path (startKeepAlivePing is called from onConnect under the instance lock, so a throw there would abort session setup). Resulting values: idleMs pingPongInterval before after ------- ----------------- ------- ----- 1000 30000 1000 333 2000 30000 1000 666 3000 30000 1000 1000 (unchanged) 90000 30000 30000 30000 (unchanged) 0 500 1000 1000 (unchanged: floor still applies) Only idle timeouts below 3000 ms change. ------------------------------------------------------------------------------------------------ 3. ALSO WORTH DOING (optional, adjacent) ------------------------------------------------------------------------------------------------ The finding notes the values are entirely unvalidated. Two cheap, separable improvements: (a) Log a warning when the idle-derived bound actually overrides the configured cadence, so a surprised administrator has a breadcrumb: if (interval < pingPongInterval) { LOG.warning(String.format( "The configured heartbeat interval %d ms exceeds the bound derived from the web " + "socket idle timeout %d ms; using %d ms instead.", pingPongInterval, idleMs, interval)); } (b) Reject an idle timeout that cannot support a heartbeat at all (below, say, PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL) at configuration-read time in GuiWebDriver / ChuiWebSimulator, alongside the pingPongInterval clamp those methods already do (see proposal-issue-major-functional-3, which centralises that clamp). Out of scope for this finding; note it rather than growing this change. ------------------------------------------------------------------------------------------------ 4. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-functional-2 the deadline is computed from this interval (dataIdleMs = max(idleMs, interval * DATA_QUIET_MULTIPLIER)); with idleMs=1000 the deadline becomes max(1000, 999) = 1000 - correct, and the ping now has a 2/3 margin inside it proposal-issue-minor-functional-5 the javadoc sentence asserting this invariant is in the same block and must be re-checked once the order is fixed (after this change the assertion becomes true as written) ------------------------------------------------------------------------------------------------ 5. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: none for any configuration with an idle timeout >= 3000 ms, which is every configuration shipped in this workspace and the 90000 ms default. Verification 1. Table-test the interval computation. This is the strongest argument for extracting it into a package-private static helper (see proposal-issue-major-functional-2 section 4), since the whole defect is arithmetic and there is currently no test for it. Cases: idleMs in {0, 1, 2, 999, 1000, 2000, 3000, 90000, 600000} x pingPongInterval in {1, 1000, 30000}. 2. Run the Web GUI with client/web/socketTimeout=1000: the startup log must report interval=333 and the session must stay up (before: interval=1000, reconnect loop). 3. Confirm no IllegalArgumentException from Timer.schedule with socketTimeout=1 and =2.