FWD » Core Development » Runtime Infrastructure

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

PROPOSAL - [MAJOR] functional - WebPageHandler.WebPageKeysProvider

PING_PONG_INTERVAL is exported to the page unclamped, so the two ends disagree

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

REVIEW FINDING (abridged)

   "the PING_PONG_INTERVAL template supplier emits the raw configured value unclamped, while both Java

   readers changed by this revision (GuiWebDriver.init and ChuiWebSimulator) now sanitise

   pingPongInterval <= 0 to the 30000 default, so the two sides of the same heartbeat derive from

   different numbers. [...] The server pings every 30 s; the page computes pingPongInterval =

   Math.max(0, 100) = 100 and maxLostPings = 2, so checkServerSilence/checkLostPings tear a healthy

   socket down roughly 300 ms after every connect, giving a permanent reconnect loop. [...] This is

   also a regression: pre-revision the same misconfiguration merely made the client ping every 100 ms

   and the socket stayed up. AUTH_COOKIE_TTL and CLIENT_RESPONSE_TIMEOUT in the same provider are

   clamped for exactly this reason - the clamp belongs in one place so all three readers agree."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/WebPageHandler.java            (primary)

   src/com/goldencode/p2j/web/WebConfigurationConstants.java                 (shared helper)

   src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebDriver.java          (de-duplicate)

   src/com/goldencode/p2j/ui/client/chui/driver/web/ChuiWebSimulator.java     (de-duplicate)

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

1. ROOT CAUSE

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

Three readers of one key, two of them clamping and one not:

   WebPageHandler.java:251-252      raw   config.getInt(PING_PONG_INTERVAL, 30000)     -> the page

   GuiWebDriver.java:652-658        clamped <= 0 to 30000                              -> the server

   ChuiWebSimulator.java:166-172    clamped <= 0 to 30000                              -> the server

r16675 introduced the two clamps and left the export alone, so a non-positive configured value now

produces a server that pings every 30 s and a page that expects to hear something every 100 ms:

   p2j.socket.js   pingPongInterval = Math.max(0, 100) = 100

                   maxLostPings     = Math.max(trunc(maxIdleTime / (2 * 100)), 2)  -> 2 when

                                      neither socketTimeout nor maxIdleTime is configured

                                      (maxIdleTime exports as CONNECTION_TIMEOUT = -1)

                   checkServerSilence fires every 100 ms, checkLostPings tears the socket down once

                   lostPings > 2

i.e. teardown ~300 ms after every connect, then attemptToRestoreConnection() - a permanent reconnect

loop. Reachable two ways, both plausible: pingPongInterval=0 (the "disable the heartbeat" attempt the

new Java comment explicitly anticipates) and pingPongInterval=-1 (the "unset" convention used by the

neighbouring items socketTimeout, watchdogTimeout and broadcastChannelPingTimeout in the same

directory section). Nothing validates the value - dir_schema.xml has no client/web object class - so

it is accepted verbatim from directory.xml.

Pre-r16675 the same misconfiguration was benign: the page pinged every 100 ms and nothing tore the

socket down. This is therefore a regression, not a pre-existing wart.

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

2. PROPOSED FIX

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

Put the clamp in one place and have all three readers call it. Duplicating the same "if (x <= 0)"

block in a third site would leave the next reader free to forget it again.

2.1 One accessor in WebConfigurationConstants

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

WebConfigurationConstants is an interface of constants; add the clamp as a static helper next to the

constant it defends (or, if a constants-only interface is preferred by convention, put the helper on

WebPageHandler and call it from the two drivers - the important property is one implementation):

   /** The default heartbeat interval, at which the server pings the client and expects an echo back */

   int PING_PONG_INTERVAL = 30000;

   /**

    * Sanitise a configured heartbeat interval.

    * <p>

    * A non-positive value reads as "disable the heartbeat", but there is no such mode: the interval

    * also derives the dead peer deadline on the server and the silence watchdog on the page. All

    * readers of {@code client/web/pingPongInterval} must clamp through here, so that the value the

    * page is given and the value the server pings at can never diverge.

    *

    * @param    interval

    *           The configured interval, in milliseconds.

    *

    * @return   The interval to use, always positive.

    */

   static int sanitizePingPongInterval(int interval)

   {

      return (interval > 0) ? interval : PING_PONG_INTERVAL;

   }

2.2 WebPageHandler: clamp the export

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

WebPageHandler.java:251-252:

   -  add(ConfigItem.PING_PONG_INTERVAL.name(), () -> String.valueOf(

   -        config.getInt(ConfigItem.PING_PONG_INTERVAL, WebConfigurationConstants.PING_PONG_INTERVAL)));

   +  // heartbeat interval in milliseconds; clamp non-positive values to the default so the page's

   +  // silence watchdog and the server's ping cadence are derived from the same number

   +  add(ConfigItem.PING_PONG_INTERVAL.name(), () -> String.valueOf(

   +        WebConfigurationConstants.sanitizePingPongInterval(

   +           config.getInt(ConfigItem.PING_PONG_INTERVAL,

   +                         WebConfigurationConstants.PING_PONG_INTERVAL))));

This matches the shape of the two neighbouring clamped suppliers (AUTH_COOKIE_TTL at :256-260,

CLIENT_RESPONSE_TIMEOUT at :263-268) added for exactly this reason.

2.3 The two drivers: call the shared helper

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

GuiWebDriver.java:652-658:

   -  pingPongInterval = config.getInt(ConfigItem.PING_PONG_INTERVAL, PING_PONG_INTERVAL);

   -  // 0 reads as "disable the heartbeat" but there is no such mode: the interval also derives the

   -  // dead peer deadline, and MIN_PING_INTERVAL would floor it to a 1s ping and a 3s deadline

   -  if (pingPongInterval <= 0)

   -  {

   -     pingPongInterval = PING_PONG_INTERVAL;

   -  }

   +  pingPongInterval = WebConfigurationConstants.sanitizePingPongInterval(

   +        config.getInt(ConfigItem.PING_PONG_INTERVAL, PING_PONG_INTERVAL));

ChuiWebSimulator.java:166-172: the identical replacement. This also disposes of the style finding

about the missing blank line after the GuiWebDriver block, since the block itself is gone.

2.4 Optional hardening: an upper bound too

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

An absurdly large value is also accepted today (WebClientBuilderParameters.java:107 already puts

20 * 60 * 1000 = 20 min). Nothing breaks - the server clamps the interval down to idleMs / 3 - so this

is optional, but if a bound is wanted, clamp to the idle tolerance in one place rather than in each

reader.

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

3. WHY NOT "MAKE THE PAGE TOLERANT INSTEAD"

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

An alternative is to floor pingPongInterval inside p2j.socket.js (it already does

Math.max(0, cfg...)). That is strictly worse: it leaves three independent derivations of one key and

the divergence would simply reappear at a different value. The page should receive a value it can

trust; the clamp belongs at the single point where the configuration is read.

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

4. RISK AND VERIFICATION

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

Risk: negligible. The only behaviour change is for values that are currently broken. Sites that set 0

or -1 today are in a reconnect loop and will start working.

Verification

   1. Set client/web/pingPongInterval to 0 in a test directory.xml, launch the Web GUI: the page

      config must show 30000, the socket must stay up, and the server log must report

      "Keep-alive ping started with interval=30000".

   2. Repeat with -1 (the neighbouring items' "unset" convention).

   3. Repeat with a valid value (10000) and confirm nothing changed.

   4. Same three cases in web CHUI (ChuiWebSimulator path).

   5. Grep for any further readers of ConfigItem.PING_PONG_INTERVAL before landing, so the "one

      implementation" property holds: currently WebPageHandler, GuiWebDriver, ChuiWebSimulator,

      WebClientBuilderOptions.java:309 and WebClientSpawner.java:644 (the last two only forward the

      raw option to the spawned client, which then reads it through the clamped paths).