================================================================================================ 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. *

* 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).