================================================================================================ PROPOSAL - [MAJOR] functional - WebClientProtocol.startKeepAlivePing Dead-peer deadline derived from pingPongInterval, so the configured idle tolerance is ignored ================================================================================================ REVIEW FINDING (abridged) "the dead-peer deadline is hard-coded at DATA_QUIET_MULTIPLIER * interval where interval = min(pingPongInterval, idleMs / PING_INTERVAL_DIVISOR), so whenever pingPongInterval < idleMs/3 the configured idle tolerance is ignored entirely and teardown is governed solely by client/web/pingPongInterval - silently converting that key from 'how often to ping' into 'how long may the peer be silent'. There is no independent directory key for the deadline and no way to disable the check. [...] hotel_gui (maxIdleTime=600000, pingPongInterval=10000) yields a 30 s server deadline against a configured 10-minute tolerance [...] counteract (maxIdleTime=3600000, pingPongInterval=30000) yields 90 s against a one-hour tolerance." FILES src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java (optional part 2.3: ConfigItem.java, WebConfigurationConstants.java, GuiWebDriver.java, ChuiWebSimulator.java, GuiWebSocket.java, StorageMessagingWebSocket.java, WebClientBuilderOptions.java, WebClientSpawner.java) ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ WebClientProtocol.java:1664: final long dataIdleMs = interval * DATA_QUIET_MULTIPLIER; The deadline is a multiple of the ping *cadence*. Cadence and tolerance are independent settings: how often the server asks has nothing to do with how long silence should be forgiven. Because interval is already clamped down to idleMs / 3, dataIdleMs can never exceed idleMs - and whenever pingPongInterval is the smaller of the two (the common case) the configured tolerance vanishes from the calculation entirely. The result is a silent redefinition of an existing directory key. Administrators who set pingPongInterval to get a livelier heartbeat now get a *shorter* death sentence, and the browser does not agree with them either: p2j.socket.js still derives its own tolerance from maxIdleTime (maxLostPings = max(trunc(maxIdleTime / (2 * pingPongInterval)), 2)), so the two ends of the same heartbeat believe in different deadlines. Shipped configurations in this workspace: hotel_gui/deploy/server/directory.xml maxIdleTime=600000 pingPongInterval=10000 no socketTimeout server deadline 30 s vs. configured 10 min (browser believes ~5 min) counteract/deploy/server/directory.xml maxIdleTime=3600000 pingPongInterval=30000 server deadline 90 s vs. configured 1 h Neither has any way to opt out: startKeepAlivePing is called unconditionally from onConnect, and both drivers coerce a configured 0 or negative pingPongInterval back to 30000. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Derive the deadline from the idle tolerance (recommended, minimal) ----------------------------------------------------------------------- Replace WebClientProtocol.java:1663-1664 with: - // the deadline must outlast the ping interval, which is what feeds it, not the idle timeout - final long dataIdleMs = interval * DATA_QUIET_MULTIPLIER; + // The deadline is the configured idle tolerance, not a multiple of the ping cadence. The + // cadence says how often the peer is asked; the idle timeout says how long its silence is + // acceptable, and before this heartbeat existed the idle timeout was exactly what bounded peer + // silence - so honouring it here keeps client/web/pingPongInterval meaning "how often to ping" + // and nothing more. The DATA_QUIET_MULTIPLIER floor keeps a couple of lost or slowly answered + // pings from costing a healthy session where the idle tolerance is short or absent. + final long dataIdleMs = Math.max(idleMs, interval * DATA_QUIET_MULTIPLIER); and retarget the DATA_QUIET_MULTIPLIER javadoc from "the deadline" to "the floor of the deadline": /** * The multiplier applied to the ping interval to obtain the *floor* of the deadline by which the * peer must have answered. It is deliberately larger than 1, so that a couple of lost or slowly * answered pings do not cost a healthy session where no idle timeout is configured, or where the * configured one is shorter than a few heartbeats. */ Resulting behaviour: idleMs pingPongInterval interval deadline (was -> now) ------- ----------------- --------- --------------------- 600000 10000 10000 30000 -> 600000 (hotel_gui: matches configuration) 3600000 30000 30000 90000 -> 3600000 (counteract: matches configuration) 90000 30000 30000 90000 -> 90000 (default: unchanged) 10000 30000 3333 9999 -> 10000 (socketTimeout=10000: honoured) 0 30000 30000 90000 -> 90000 (no idle timeout: floor applies) This restores pre-r16675 semantics exactly (peer silence bounded by the configured idle tolerance) while keeping the property the heartbeat was introduced for: the check still runs when there is no idle timeout at all, which is where Jetty would never reap a half-open socket. It also re-aligns the two ends of the heartbeat: the browser's maxLostPings is derived from maxIdleTime, so both sides now scale with the same key. 2.2 Fix the javadoc claims that go with it ------------------------------------------- The method javadoc currently asserts the interval "tracks client/web/pingPongInterval, so that one setting governs the heartbeat cadence instead of two drifting apart", and that the idle timeout "is only ever used to clamp the interval". After 2.1 the idle timeout also sets the deadline; say so, and state the resulting worst-case teardown time (see proposal-issue-minor-security-1, which owns the worst-case wording). Also fix the wrong key name in the same block (proposal-issue-minor-functional-5). 2.3 Optional: an explicit key, including an off switch ------------------------------------------------------ 2.1 removes the surprise but still offers no way to disable the check, which the finding flags. If an off switch is wanted, add a dedicated key rather than overloading pingPongInterval further: ConfigItem.java, next to PING_PONG_INTERVAL: /** * The maximum time in milliseconds the web client may stay silent before the server closes its * web socket. Unset (or non-positive) means "derive it from the idle tolerance"; see * WebClientProtocol.startKeepAlivePing. */ public static final ConfigItem HEARTBEAT_DEADLINE = new ConfigItem<>(Integer.class, "heartbeatDeadline", "client", "web", "heartbeatDeadline", Type.WEB_CLIENT); WebConfigurationConstants: /** The default heartbeat deadline; -1 means derive it from the web socket idle tolerance */ int HEARTBEAT_DEADLINE = -1; Plumbing mirrors pingPongInterval exactly, which r16675 already established: GuiWebDriver.init / ChuiWebSimulator read it (keeping -1 rather than coercing, since -1 is the documented "unset" convention of the neighbouring items) -> GuiWebSocket / StorageMessagingWebSocket constructors -> WebClientProtocol constructor field; WebClientBuilderOptions.java:307-309 and WebClientSpawner.java:638-644 must pass it through for spawned clients, or a spawned client will silently fall back to the derived value. In startKeepAlivePing: final long dataIdleMs = (heartbeatDeadline > 0) ? Math.max(heartbeatDeadline, interval * DATA_QUIET_MULTIPLIER) : Math.max(idleMs, interval * DATA_QUIET_MULTIPLIER); For a hard off switch, treat an explicit 0 as "no dead-peer check" and skip the deadline branch entirely (still sending the pings, which are what defeat Jetty's idle timeout). Document loudly that this reinstates the half-open-socket linger the heartbeat exists to prevent. Recommendation: land 2.1 + 2.2 now; treat 2.3 as a follow-up only if a site actually needs it. 2.1 alone removes every mismatch found in the shipped configurations, and it does so without adding a directory key that has no schema validation behind it (dir_schema.xml has no client/web object class - see proposal-issue-minor-functional-1). ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-functional-1 the outbound grace; complementary - this fixes the deadline's magnitude, that one fixes what evidence can postpone it proposal-issue-minor-functional-1 clamp order for interval, which feeds the floor computed here proposal-issue-minor-functional-5 the wrong key name in the same javadoc block proposal-issue-minor-functional-6 ConfigItem.MAX_IDLE_TIME javadoc, which must be corrected in the same breath: after 2.1 the key does bound teardown again, for WebClientProtocol websockets ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: a long configured maxIdleTime now means a long dead-peer detection window again - e.g. one hour on counteract. That is the administrator's stated intent and the pre-r16675 behaviour, and the watchdog (client-side, 120 s default) still covers the abandoned-client case. Sites wanting fast reclamation should shorten maxIdleTime / socketTimeout, which is what those keys have always meant. Verification 1. Unit-level: extract the interval/deadline computation into a package-private static helper (long computeDeadline(long idleMs, long pingPongInterval)) and table-test the matrix in 2.1. There is currently no test coverage of any of this arithmetic. 2. Run with hotel_gui/deploy/server/directory.xml: startup log must read "interval=10000, idle timeout=600000, heartbeat deadline=600000". 3. Suspend the client host for 5 minutes with the counteract configuration: session survives (previously lost at ~90 s + watchdog). 4. Kill the browser process outright with the default configuration: teardown still at 90 s.