================================================================================================ PROPOSAL - [MINOR] functional - ConfigItem.MAX_IDLE_TIME (javadoc) The rewritten javadoc states unconditionally a property that holds only for one websocket type ================================================================================================ REVIEW FINDING (abridged) "the rewritten javadoc states as unconditional ('a connection is no longer closed on idle time alone') a property that holds only for websockets descended from WebClientProtocol, which is where startKeepAlivePing lives. The same client/web/maxIdleTime key is read by WebSocketConfig and applied by GenericWebServer.addWebSocketHandler as the container idle timeout for *every* websocket registered that way, including ReportProtocol on /api (whose ReportWebServer sets the key to 36000000) and NetworkTestSocket - neither extends WebClientProtocol and neither runs the keep-alive, so for those endpoints the key is still a plain idle timeout. The javadoc also omits that the key is exported to the browser, where it still drives maxLostPings and the maxLifeTime of both ControlTimers in p2j.socket.js. [...] (Relatedly, the claim that this item 'bounds the keep-alive ping interval' is imprecise: a positive client/web/socketTimeout takes precedence, and the spawner supplies one in every spawned client.)" FILE src/com/goldencode/p2j/util/ConfigItem.java (comments only) ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ ConfigItem.java:1358-1362 as changed by this revision: /** * The websocket idle time. Note the server side keep-alive writes reset Jetty's idle clock, so a * connection is no longer closed on idle time alone; this instead bounds the keep-alive ping * interval, and teardown is governed by the heartbeat deadline derived from it. */ Three inaccuracies, each verifiable in the tree: (a) "a connection is no longer closed on idle time alone" is scoped to the wrong thing. The key is read once, generically, by WebSocketConfig.java:97 and applied by GenericWebServer.java:948 (`webSocketHandler.setIdleTimeout(...)`) to *every* websocket registered through addWebSocketHandler. Registrations in the tree: /api ReportProtocol ReportWebServer.java:151 (and ReportWebServer.java:148 sets this very key to 36000000, "10 hours") NETWORK_TEST_TARGET NetworkTestSocket EmbeddedWebServerImpl.java:273 AJAX_TARGET the web client EmbeddedWebServerImpl.java:271 Only the third descends from WebClientProtocol and runs startKeepAlivePing. For the other two the key is still exactly what it always was: a plain idle timeout that does close the connection. (b) The client-side role is omitted. The key is exported to the page by WebPageHandler.WebPageKeysProvider (WebPageHandler.java:205-225), where it drives maxLostPings (`Math.max(trunc(maxIdleTime / (2 * pingPongInterval)), 2)`) and the maxLifeTime of the ControlTimers in p2j.socket.js (`new ControlTimer(Math.max(watchdogTimeout, maxIdleTime), ...)`). An administrator changing this key changes the browser's own silence tolerance too, which is not something the javadoc lets them guess. (c) "this instead bounds the keep-alive ping interval" is imprecise about precedence. The interval is clamped from Session.getIdleTimeout(), which is this key *only when* client:web:socketTimeout is not positive (WebPageHandler.java:207-222 documents the same precedence: "if socketTimeout > 0, then it overrides maxIdleTime"). And socketTimeout is positive in every spawned client: WebClientBuilderOptions.java:294 adds it with MAX_WEB_SOCKET_IDLE_TIMEOUT (90000) and WebClientSpawner.java:633 passes it on. So in the common deployment this key does *not* bound the ping interval at all. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ Comment-only. Replace the javadoc with one that scopes each claim: /** * The websocket idle time, in milliseconds, applied by * {@code GenericWebServer.addWebSocketHandler} to every websocket it registers. *

* For the web client websocket ({@code WebClientProtocol} and its subclasses) it is no longer the * thing that closes an idle connection: that protocol runs a server driven keep-alive whose writes * reset Jetty's idle clock, so teardown is governed by its own dead peer deadline. This item is * what sizes that deadline, and it also bounds the keep-alive ping interval - but only where * {@link #WEB_SOCKET_TIMEOUT} is not positive, since a positive one takes precedence, and the * spawner supplies one for every spawned client. *

* For every other websocket registered the same way - {@code ReportProtocol} on {@code /api}, * {@code NetworkTestSocket} - none of that applies: they do not run the keep-alive, so this remains * a plain idle timeout which does close the connection. *

* The value is also exported to the browser, where it still derives the client's own tolerance for * server silence ({@code maxLostPings}) and the lifetime of the connectivity and idle timers in * {@code p2j.socket.js}. Shortening it therefore shortens both ends of the heartbeat, not just the * server's. */ Line lengths are within the 110-column limit used by the file; the {@link #WEB_SOCKET_TIMEOUT} reference is in the same class so no import is needed. 2.1 If proposal-issue-major-functional-2 lands ----------------------------------------------- That proposal makes the deadline `Math.max(idleMs, interval * DATA_QUIET_MULTIPLIER)`, i.e. this key becomes the deadline directly rather than merely sizing it. The middle sentence then reads: * ... This item *is* that deadline (floored at a few ping intervals so a couple of lost pings do not * cost a healthy session), and it also bounds the keep-alive ping interval - but only where * {@link #WEB_SOCKET_TIMEOUT} is not positive ... which is both simpler and closer to the pre-heartbeat meaning of the key. Write the javadoc once, after deciding on that proposal, rather than twice. 2.2 Adjacent, optional ----------------------- PING_PONG_INTERVAL's rewritten javadoc one declaration above (ConfigItem.java:1353) is accurate as far as it goes ("The heartbeat interval, at which the server pings the client and expects an echo back") but omits that a non-positive value is coerced to the default and that the page derives its silence watchdog from the same value. If proposal-issue-major-functional-3 lands (one clamp, one place), add a sentence naming the clamp so the three readers' shared contract is documented where the key is declared. ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-functional-2 changes what this key actually does; land first, then write this javadoc once (see 2.1) proposal-issue-minor-functional-5 the same class of error in the WebClientProtocol javadoc; fix both in one pass so the two descriptions agree proposal-issue-major-functional-3 supplies the clamp referenced by 2.2 ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: none - comments only. Verification 1. `./gradlew javadoc` produces no new warning (the {@link} target is in the same class). 2. Re-read the finished text against the three registration sites listed in section 1 and confirm each claim is scoped: web client / other websockets / browser. 3. Sanity check the precedence claim by launching a spawned client and confirming the effective idle timeout in the "Keep-alive ping started with interval=..., idle timeout=..." log line matches client:web:socketTimeout, not client:web:maxIdleTime.