FWD » Core Development » Runtime Infrastructure

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

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