FWD » Core Development » Runtime Infrastructure

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

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<Integer> 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.