================================================================================================ PROPOSAL - [MINOR] functional - ConfigItem.MAX_IDLE_TIME The added javadoc sentence describes a page export that no shipped profile performs ================================================================================================ REVIEW FINDING (abridged) "The added sentence 'Also exported to the page, where it sizes the browser's own silence tolerance' does not describe any shipped configuration [...] WebClientBuilderOptions.readConfigs unconditionally emits webSocketTimeout (default 90000; the debug profile sets 1200000) [...] so the maxIdleTime branch is dead in both shipped profiles." FILES src/com/goldencode/p2j/util/ConfigItem.java ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ Current text, ConfigItem.java:361-367: /** * The idle time after which the connection should be closed, if webSocketTimeout is not set. Also * exported to the page, where it sizes the browser's own silence tolerance. */ public static final ConfigItem MAX_IDLE_TIME = ... The caveat "if webSocketTimeout is not set" governs only the first sentence. The second sentence adds a second role with no caveat - and it is the role an administrator would actually tune, since it is what decides how long the browser tolerates silence before tearing the socket down. Both roles are in fact subordinate to webSocketTimeout: - WebPageHandler's MAX_IDLE_TIME key reads WEB_SOCKET_TIMEOUT first and returns it verbatim when positive, falling back to MAX_IDLE_TIME only in the else. The new PING_PONG_INTERVAL key has the same precedence. - WebClientBuilderOptions.readConfigs does add(options, WEB_SOCKET_TIMEOUT, MAX_WEB_SOCKET_IDLE_TIMEOUT) unconditionally (default 90000), and WebClientSpawner appends it to the spawned client's command line with no null check. The debug profile in WebClientBuilderParameters sets it to 1200000. So the fallback branch is unreachable in both shipped profiles. The wording is also imprecise in a second way: the code branches on webSocketTimeout > 0, not on the key being absent. "Not set" suggests omitting it from directory.xml is enough, when in fact an explicit non-positive value is required to reach the maxIdleTime branch at all. Concrete trigger: set client:web:maxIdleTime=300000 and leave socketTimeout alone. index.html renders ${maxIdleTime}=90000, and the browser's silence tolerance ignores the configured value with nothing in the log to say why. In fairness, maxIdleTime is not inert - WebSocketConfig feeds it to GenericWebServer as the websocket handler's default idle timeout. But WebClientProtocol.onConnect then overrides that per session from webSocketTimeout, so the override applies consistently to both the container timeout and the page export. That is precisely why the caveat belongs on both sentences. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ Make one caveat govern both roles and name the actual condition: /** * The idle time after which the connection should be closed, and the silence tolerance exported * to the page. In both roles this applies only while {@code client:web:webSocketTimeout} is not * positive; otherwise that setting supersedes it, and a spawned web client always receives an * explicit value for it. */ public static final ConfigItem MAX_IDLE_TIME = new ConfigItem<>(Integer.class, "maxIdleTime", "client", "web", "maxIdleTime", Type.WEB_CLIENT); The final clause is the part that saves an administrator's afternoon: it says outright that in practice this value is not the one in force. Note the deliberate choice of the client:web: notation, matching WATCHDOG_TIMEOUT's neighbouring doc and the corrected notation used elsewhere in this revision. The file's history entry 020 ("Corrected the heartbeat related config docs") already covers this work; no new entry is needed if it lands in the same commit. 2.1 Optional companion, not proposed here ------------------------------------------- The underlying usability problem is that two keys silently shadow each other with no diagnostic. A one-line INFO in WebClientProtocol.onConnect or WebPageHandler, emitted when both are configured and webSocketTimeout wins, would make the shadowing visible at runtime rather than only in javadoc. That is a behaviour change and belongs in its own task; it is recorded here because the same shadowing was the substance of a second, rejected review finding about PING_PONG_INTERVAL, which suggests the pattern rather than the individual doc is what deserves attention. ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ None. Self-contained comment change. Related but not proposed: the review's PING_PONG_INTERVAL javadoc finding was rejected on the grounds that no other ConfigItem in the file documents clamping. If 2.1 is ever taken, that decision is worth revisiting for both keys together. ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: none. Comment-only. Verification 1. ./gradlew compile and ./gradlew javadoc produce no new warnings. 2. Confirm the claim empirically before committing the wording: set client:web:maxIdleTime to a distinctive value (300000), leave socketTimeout unset, start a web client, and check the rendered index.html - ${maxIdleTime} should be 90000, not 300000. 3. Repeat with client:web:socketTimeout explicitly set to 0 and confirm the maxIdleTime value does then reach the page, i.e. the branch is reachable exactly as the new wording says.