================================================================================================ PROPOSAL - [MINOR] functional - WebClientProtocol.startKeepAlivePing (javadoc) The javadoc names client/web/webSocketTimeout, which is not a valid path for either mechanism ================================================================================================ REVIEW FINDING (abridged) "the new javadoc names client/web/webSocketTimeout, which is not a valid path for either configuration mechanism. Per ConfigItem's constructor contract the 2nd argument is the *directory node name* and arguments 3-5 are the bootstrap option category/group/key, so WEB_SOCKET_TIMEOUT has directory node webSocketTimeout (resolved under the webClient/ or clientConfig/ containers, never under client/web/) and bootstrap option client:web:socketTimeout. Either way an administrator following this javadoc would add a node that is never read. [...] Note also that the slash notation is introduced by this diff and appears nowhere else in the tree - every pre-existing reference uses the colon form - so it cannot be defended as an accepted informal spelling." FILE src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java (comments only) ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ ConfigItem.java:374-375: public static final ConfigItem WEB_SOCKET_TIMEOUT = new ConfigItem<>(Integer.class, "webSocketTimeout", "client", "web", "socketTimeout", Type.WEB_CLIENT); Argument 2 is the directory node name; arguments 3-5 are the bootstrap option category, group and key. So the two valid spellings are the directory node `webSocketTimeout` and the bootstrap option `client:web:socketTimeout`. The composite `client/web/webSocketTimeout` in the javadoc mixes the container prefix of one with the leaf of the other and matches neither - an administrator following it adds a node nothing ever reads, and gets no error for their trouble. The two sibling names in the same sentence, `client/web/maxIdleTime` and `client/web/pingPongInterval`, *do* map onto real bootstrap options (only the separator is wrong), which is exactly what makes the odd one out easy to trust. Notation: `client/web/` appears in the tree only on the three lines this diff introduced (WebClientProtocol.java:317, :1627, :1634). Every pre-existing reference uses the colon form - e.g. ConfigItem.java:378 "client:web:clientResponseTimeout", :390 "client:web:authCookieTtl", WebClientBuilder.java:16 "client:web:embedded". ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ Comment-only. Three sites, and the notation converted to the established colon form throughout. 2.1 The field javadoc (WebClientProtocol.java:316-318) ------------------------------------------------------ - * The configured heartbeat interval, from {@code client/web/pingPongInterval}. Both the interval at - * which {@link #MSG_SERVER_PING} is sent and the deadline for its echo are derived from it. + * The configured heartbeat interval, from {@code client:web:pingPongInterval}. Both the interval at + * which {@link #MSG_SERVER_PING} is sent and the deadline for its echo are derived from it. (If proposal-issue-major-functional-2 lands, the second sentence must also change: the deadline is then derived from the idle tolerance, not from this interval.) 2.2 The interval paragraph (WebClientProtocol.java:1627) -------------------------------------------------------- - * The interval tracks {@code client/web/pingPongInterval}, so that one setting governs the + * The interval tracks {@code client:web:pingPongInterval}, so that one setting governs the 2.3 The idle-timeout paragraph (WebClientProtocol.java:1632-1636) - the actual error ------------------------------------------------------------------------------------ - * The idle timeout is read from {@link Session#getIdleTimeout}, not from the configured web - * socket timeout, because it may also come from the container level - * {@code client/web/maxIdleTime} when {@code client/web/webSocketTimeout} is not positive. It is - * only ever used to clamp the interval: ... + * The idle timeout is read from {@link Session#getIdleTimeout}, not from the configured web + * socket timeout, because it may also come from the container level + * {@code client:web:maxIdleTime} when {@code client:web:socketTimeout} (directory node + * {@code webSocketTimeout}, see {@link ConfigItem#WEB_SOCKET_TIMEOUT}) is not positive. It is + * only ever used to clamp the interval: ... Naming the ConfigItem in a {@link} is the part that stops this from rotting again: the two spellings of one key are not derivable from each other, and a link fails at compile time if the item is renamed, whereas a hand-written path does not. 2.4 Check the rest of the change for the same slip -------------------------------------------------- Before landing, grep the whole diff for the slash form so no fourth instance is missed: grep -n "client/web/" $(brz status --short | awk '{print $2}') At the time of review the only occurrences are the three above. The p2j.socket.js and p2j.sso_reauth.js comments added by the same revision refer to keys only by their exported JS names (pingPongInterval, maxIdleTime), which is correct in that context. ------------------------------------------------------------------------------------------------ 3. CONSISTENCY NOTE (worth deciding once) ------------------------------------------------------------------------------------------------ The tree has two ways to name the same setting and no stated convention: bootstrap option client:web:socketTimeout (what an administrator puts on a command line / in a cfg override; what most comments use) directory node webSocketTimeout (what appears in directory.xml, under webClient/ or clientConfig/) Where the distinction matters - and in this javadoc it does, because the sentence is about where the value *comes from* - name both, as 2.3 does. Where it does not, use the colon form, matching every pre-existing comment. Adding this as a sentence to ConfigItem's class javadoc would save the next author the same mistake, but that is a separate change. ------------------------------------------------------------------------------------------------ 4. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-functional-2 rewrites the surrounding paragraphs (the idle timeout stops being "only ever used to clamp the interval"); apply that first and fold these corrections into the rewritten text rather than editing the same lines twice proposal-issue-minor-functional-1 the invariant asserted two lines above ("held to one PING_INTERVAL_DIVISORth of it") is false until the clamp order is fixed; same javadoc block proposal-issue-minor-functional-6 the companion doc correction in ConfigItem.MAX_IDLE_TIME ------------------------------------------------------------------------------------------------ 5. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: none - comments only. The one substantive check is the {@link ConfigItem#WEB_SOCKET_TIMEOUT} reference: ConfigItem is in com.goldencode.p2j.util and is not currently imported by WebClientProtocol.java, so either add the import or use the fully qualified form in the link. Confirm with `./gradlew javadoc` that no new javadoc warning appears.