|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - WebClientProtocol.startKeepAlivePing (javadoc)
|
|
3
|
The javadoc names client/web/webSocketTimeout, which is not a valid path for either mechanism
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"the new javadoc names client/web/webSocketTimeout, which is not a valid path for either
|
|
8
|
configuration mechanism. Per ConfigItem's constructor contract the 2nd argument is the *directory node
|
|
9
|
name* and arguments 3-5 are the bootstrap option category/group/key, so WEB_SOCKET_TIMEOUT has
|
|
10
|
directory node webSocketTimeout (resolved under the webClient/ or clientConfig/ containers, never
|
|
11
|
under client/web/) and bootstrap option client:web:socketTimeout. Either way an administrator
|
|
12
|
following this javadoc would add a node that is never read. [...] Note also that the slash notation is
|
|
13
|
introduced by this diff and appears nowhere else in the tree - every pre-existing reference uses the
|
|
14
|
colon form - so it cannot be defended as an accepted informal spelling."
|
|
15
|
|
|
16
|
FILE
|
|
17
|
src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java (comments only)
|
|
18
|
|
|
19
|
------------------------------------------------------------------------------------------------
|
|
20
|
1. ROOT CAUSE
|
|
21
|
------------------------------------------------------------------------------------------------
|
|
22
|
|
|
23
|
ConfigItem.java:374-375:
|
|
24
|
|
|
25
|
public static final ConfigItem<Integer> WEB_SOCKET_TIMEOUT =
|
|
26
|
new ConfigItem<>(Integer.class, "webSocketTimeout", "client", "web", "socketTimeout",
|
|
27
|
Type.WEB_CLIENT);
|
|
28
|
|
|
29
|
Argument 2 is the directory node name; arguments 3-5 are the bootstrap option category, group and key.
|
|
30
|
So the two valid spellings are the directory node `webSocketTimeout` and the bootstrap option
|
|
31
|
`client:web:socketTimeout`. The composite `client/web/webSocketTimeout` in the javadoc mixes the
|
|
32
|
container prefix of one with the leaf of the other and matches neither - an administrator following it
|
|
33
|
adds a node nothing ever reads, and gets no error for their trouble.
|
|
34
|
|
|
35
|
The two sibling names in the same sentence, `client/web/maxIdleTime` and `client/web/pingPongInterval`,
|
|
36
|
*do* map onto real bootstrap options (only the separator is wrong), which is exactly what makes the odd
|
|
37
|
one out easy to trust.
|
|
38
|
|
|
39
|
Notation: `client/web/` appears in the tree only on the three lines this diff introduced
|
|
40
|
(WebClientProtocol.java:317, :1627, :1634). Every pre-existing reference uses the colon form - e.g.
|
|
41
|
ConfigItem.java:378 "client:web:clientResponseTimeout", :390 "client:web:authCookieTtl",
|
|
42
|
WebClientBuilder.java:16 "client:web:embedded".
|
|
43
|
|
|
44
|
------------------------------------------------------------------------------------------------
|
|
45
|
2. PROPOSED FIX
|
|
46
|
------------------------------------------------------------------------------------------------
|
|
47
|
|
|
48
|
Comment-only. Three sites, and the notation converted to the established colon form throughout.
|
|
49
|
|
|
50
|
2.1 The field javadoc (WebClientProtocol.java:316-318)
|
|
51
|
------------------------------------------------------
|
|
52
|
|
|
53
|
- * The configured heartbeat interval, from {@code client/web/pingPongInterval}. Both the interval at
|
|
54
|
- * which {@link #MSG_SERVER_PING} is sent and the deadline for its echo are derived from it.
|
|
55
|
+ * The configured heartbeat interval, from {@code client:web:pingPongInterval}. Both the interval at
|
|
56
|
+ * which {@link #MSG_SERVER_PING} is sent and the deadline for its echo are derived from it.
|
|
57
|
|
|
58
|
(If proposal-issue-major-functional-2 lands, the second sentence must also change: the deadline is then
|
|
59
|
derived from the idle tolerance, not from this interval.)
|
|
60
|
|
|
61
|
2.2 The interval paragraph (WebClientProtocol.java:1627)
|
|
62
|
--------------------------------------------------------
|
|
63
|
|
|
64
|
- * The interval tracks {@code client/web/pingPongInterval}, so that one setting governs the
|
|
65
|
+ * The interval tracks {@code client:web:pingPongInterval}, so that one setting governs the
|
|
66
|
|
|
67
|
2.3 The idle-timeout paragraph (WebClientProtocol.java:1632-1636) - the actual error
|
|
68
|
------------------------------------------------------------------------------------
|
|
69
|
|
|
70
|
- * The idle timeout is read from {@link Session#getIdleTimeout}, not from the configured web
|
|
71
|
- * socket timeout, because it may also come from the container level
|
|
72
|
- * {@code client/web/maxIdleTime} when {@code client/web/webSocketTimeout} is not positive. It is
|
|
73
|
- * only ever used to clamp the interval: ...
|
|
74
|
+ * The idle timeout is read from {@link Session#getIdleTimeout}, not from the configured web
|
|
75
|
+ * socket timeout, because it may also come from the container level
|
|
76
|
+ * {@code client:web:maxIdleTime} when {@code client:web:socketTimeout} (directory node
|
|
77
|
+ * {@code webSocketTimeout}, see {@link ConfigItem#WEB_SOCKET_TIMEOUT}) is not positive. It is
|
|
78
|
+ * only ever used to clamp the interval: ...
|
|
79
|
|
|
80
|
Naming the ConfigItem in a {@link} is the part that stops this from rotting again: the two spellings of
|
|
81
|
one key are not derivable from each other, and a link fails at compile time if the item is renamed,
|
|
82
|
whereas a hand-written path does not.
|
|
83
|
|
|
84
|
2.4 Check the rest of the change for the same slip
|
|
85
|
--------------------------------------------------
|
|
86
|
|
|
87
|
Before landing, grep the whole diff for the slash form so no fourth instance is missed:
|
|
88
|
|
|
89
|
grep -n "client/web/" $(brz status --short | awk '{print $2}')
|
|
90
|
|
|
91
|
At the time of review the only occurrences are the three above. The p2j.socket.js and
|
|
92
|
p2j.sso_reauth.js comments added by the same revision refer to keys only by their exported JS names
|
|
93
|
(pingPongInterval, maxIdleTime), which is correct in that context.
|
|
94
|
|
|
95
|
------------------------------------------------------------------------------------------------
|
|
96
|
3. CONSISTENCY NOTE (worth deciding once)
|
|
97
|
------------------------------------------------------------------------------------------------
|
|
98
|
|
|
99
|
The tree has two ways to name the same setting and no stated convention:
|
|
100
|
|
|
101
|
bootstrap option client:web:socketTimeout (what an administrator puts on a command line /
|
|
102
|
in a cfg override; what most comments use)
|
|
103
|
directory node webSocketTimeout (what appears in directory.xml, under webClient/ or
|
|
104
|
clientConfig/)
|
|
105
|
|
|
106
|
Where the distinction matters - and in this javadoc it does, because the sentence is about where the
|
|
107
|
value *comes from* - name both, as 2.3 does. Where it does not, use the colon form, matching every
|
|
108
|
pre-existing comment. Adding this as a sentence to ConfigItem's class javadoc would save the next author
|
|
109
|
the same mistake, but that is a separate change.
|
|
110
|
|
|
111
|
------------------------------------------------------------------------------------------------
|
|
112
|
4. RELATIONSHIP TO OTHER PROPOSALS
|
|
113
|
------------------------------------------------------------------------------------------------
|
|
114
|
|
|
115
|
proposal-issue-major-functional-2 rewrites the surrounding paragraphs (the idle timeout stops being
|
|
116
|
"only ever used to clamp the interval"); apply that first and fold
|
|
117
|
these corrections into the rewritten text rather than editing the
|
|
118
|
same lines twice
|
|
119
|
proposal-issue-minor-functional-1 the invariant asserted two lines above ("held to one
|
|
120
|
PING_INTERVAL_DIVISORth of it") is false until the clamp order is
|
|
121
|
fixed; same javadoc block
|
|
122
|
proposal-issue-minor-functional-6 the companion doc correction in ConfigItem.MAX_IDLE_TIME
|
|
123
|
|
|
124
|
------------------------------------------------------------------------------------------------
|
|
125
|
5. RISK AND VERIFICATION
|
|
126
|
------------------------------------------------------------------------------------------------
|
|
127
|
|
|
128
|
Risk: none - comments only. The one substantive check is the {@link ConfigItem#WEB_SOCKET_TIMEOUT}
|
|
129
|
reference: ConfigItem is in com.goldencode.p2j.util and is not currently imported by
|
|
130
|
WebClientProtocol.java, so either add the import or use the fully qualified form in the link. Confirm
|
|
131
|
with `./gradlew javadoc` that no new javadoc warning appears.
|