|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - ConfigItem.MAX_IDLE_TIME (javadoc)
|
|
3
|
The rewritten javadoc states unconditionally a property that holds only for one websocket type
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"the rewritten javadoc states as unconditional ('a connection is no longer closed on idle time alone')
|
|
8
|
a property that holds only for websockets descended from WebClientProtocol, which is where
|
|
9
|
startKeepAlivePing lives. The same client/web/maxIdleTime key is read by WebSocketConfig and applied
|
|
10
|
by GenericWebServer.addWebSocketHandler as the container idle timeout for *every* websocket registered
|
|
11
|
that way, including ReportProtocol on /api (whose ReportWebServer sets the key to 36000000) and
|
|
12
|
NetworkTestSocket - neither extends WebClientProtocol and neither runs the keep-alive, so for those
|
|
13
|
endpoints the key is still a plain idle timeout. The javadoc also omits that the key is exported to the
|
|
14
|
browser, where it still drives maxLostPings and the maxLifeTime of both ControlTimers in
|
|
15
|
p2j.socket.js. [...] (Relatedly, the claim that this item 'bounds the keep-alive ping interval' is
|
|
16
|
imprecise: a positive client/web/socketTimeout takes precedence, and the spawner supplies one in every
|
|
17
|
spawned client.)"
|
|
18
|
|
|
19
|
FILE
|
|
20
|
src/com/goldencode/p2j/util/ConfigItem.java (comments only)
|
|
21
|
|
|
22
|
------------------------------------------------------------------------------------------------
|
|
23
|
1. ROOT CAUSE
|
|
24
|
------------------------------------------------------------------------------------------------
|
|
25
|
|
|
26
|
ConfigItem.java:1358-1362 as changed by this revision:
|
|
27
|
|
|
28
|
/**
|
|
29
|
* The websocket idle time. Note the server side keep-alive writes reset Jetty's idle clock, so a
|
|
30
|
* connection is no longer closed on idle time alone; this instead bounds the keep-alive ping
|
|
31
|
* interval, and teardown is governed by the heartbeat deadline derived from it.
|
|
32
|
*/
|
|
33
|
|
|
34
|
Three inaccuracies, each verifiable in the tree:
|
|
35
|
|
|
36
|
(a) "a connection is no longer closed on idle time alone" is scoped to the wrong thing. The key is read
|
|
37
|
once, generically, by WebSocketConfig.java:97 and applied by GenericWebServer.java:948
|
|
38
|
(`webSocketHandler.setIdleTimeout(...)`) to *every* websocket registered through
|
|
39
|
addWebSocketHandler. Registrations in the tree:
|
|
40
|
|
|
41
|
/api ReportProtocol ReportWebServer.java:151
|
|
42
|
(and ReportWebServer.java:148 sets this very key to 36000000, "10 hours")
|
|
43
|
NETWORK_TEST_TARGET NetworkTestSocket EmbeddedWebServerImpl.java:273
|
|
44
|
AJAX_TARGET the web client EmbeddedWebServerImpl.java:271
|
|
45
|
|
|
46
|
Only the third descends from WebClientProtocol and runs startKeepAlivePing. For the other two the
|
|
47
|
key is still exactly what it always was: a plain idle timeout that does close the connection.
|
|
48
|
|
|
49
|
(b) The client-side role is omitted. The key is exported to the page by
|
|
50
|
WebPageHandler.WebPageKeysProvider (WebPageHandler.java:205-225), where it drives
|
|
51
|
maxLostPings (`Math.max(trunc(maxIdleTime / (2 * pingPongInterval)), 2)`) and the maxLifeTime of the
|
|
52
|
ControlTimers in p2j.socket.js (`new ControlTimer(Math.max(watchdogTimeout, maxIdleTime), ...)`).
|
|
53
|
An administrator changing this key changes the browser's own silence tolerance too, which is not
|
|
54
|
something the javadoc lets them guess.
|
|
55
|
|
|
56
|
(c) "this instead bounds the keep-alive ping interval" is imprecise about precedence. The interval is
|
|
57
|
clamped from Session.getIdleTimeout(), which is this key *only when* client:web:socketTimeout is not
|
|
58
|
positive (WebPageHandler.java:207-222 documents the same precedence: "if socketTimeout > 0, then it
|
|
59
|
overrides maxIdleTime"). And socketTimeout is positive in every spawned client:
|
|
60
|
WebClientBuilderOptions.java:294 adds it with MAX_WEB_SOCKET_IDLE_TIMEOUT (90000) and
|
|
61
|
WebClientSpawner.java:633 passes it on. So in the common deployment this key does *not* bound the
|
|
62
|
ping interval at all.
|
|
63
|
|
|
64
|
------------------------------------------------------------------------------------------------
|
|
65
|
2. PROPOSED FIX
|
|
66
|
------------------------------------------------------------------------------------------------
|
|
67
|
|
|
68
|
Comment-only. Replace the javadoc with one that scopes each claim:
|
|
69
|
|
|
70
|
/**
|
|
71
|
* The websocket idle time, in milliseconds, applied by
|
|
72
|
* {@code GenericWebServer.addWebSocketHandler} to every websocket it registers.
|
|
73
|
* <p>
|
|
74
|
* For the web client websocket ({@code WebClientProtocol} and its subclasses) it is no longer the
|
|
75
|
* thing that closes an idle connection: that protocol runs a server driven keep-alive whose writes
|
|
76
|
* reset Jetty's idle clock, so teardown is governed by its own dead peer deadline. This item is
|
|
77
|
* what sizes that deadline, and it also bounds the keep-alive ping interval - but only where
|
|
78
|
* {@link #WEB_SOCKET_TIMEOUT} is not positive, since a positive one takes precedence, and the
|
|
79
|
* spawner supplies one for every spawned client.
|
|
80
|
* <p>
|
|
81
|
* For every other websocket registered the same way - {@code ReportProtocol} on {@code /api},
|
|
82
|
* {@code NetworkTestSocket} - none of that applies: they do not run the keep-alive, so this remains
|
|
83
|
* a plain idle timeout which does close the connection.
|
|
84
|
* <p>
|
|
85
|
* The value is also exported to the browser, where it still derives the client's own tolerance for
|
|
86
|
* server silence ({@code maxLostPings}) and the lifetime of the connectivity and idle timers in
|
|
87
|
* {@code p2j.socket.js}. Shortening it therefore shortens both ends of the heartbeat, not just the
|
|
88
|
* server's.
|
|
89
|
*/
|
|
90
|
|
|
91
|
Line lengths are within the 110-column limit used by the file; the {@link #WEB_SOCKET_TIMEOUT} reference
|
|
92
|
is in the same class so no import is needed.
|
|
93
|
|
|
94
|
2.1 If proposal-issue-major-functional-2 lands
|
|
95
|
-----------------------------------------------
|
|
96
|
|
|
97
|
That proposal makes the deadline `Math.max(idleMs, interval * DATA_QUIET_MULTIPLIER)`, i.e. this key
|
|
98
|
becomes the deadline directly rather than merely sizing it. The middle sentence then reads:
|
|
99
|
|
|
100
|
* ... This item *is* that deadline (floored at a few ping intervals so a couple of lost pings do not
|
|
101
|
* cost a healthy session), and it also bounds the keep-alive ping interval - but only where
|
|
102
|
* {@link #WEB_SOCKET_TIMEOUT} is not positive ...
|
|
103
|
|
|
104
|
which is both simpler and closer to the pre-heartbeat meaning of the key. Write the javadoc once, after
|
|
105
|
deciding on that proposal, rather than twice.
|
|
106
|
|
|
107
|
2.2 Adjacent, optional
|
|
108
|
-----------------------
|
|
109
|
|
|
110
|
PING_PONG_INTERVAL's rewritten javadoc one declaration above (ConfigItem.java:1353) is accurate as far as
|
|
111
|
it goes ("The heartbeat interval, at which the server pings the client and expects an echo back") but
|
|
112
|
omits that a non-positive value is coerced to the default and that the page derives its silence watchdog
|
|
113
|
from the same value. If proposal-issue-major-functional-3 lands (one clamp, one place), add a sentence
|
|
114
|
naming the clamp so the three readers' shared contract is documented where the key is declared.
|
|
115
|
|
|
116
|
------------------------------------------------------------------------------------------------
|
|
117
|
3. RELATIONSHIP TO OTHER PROPOSALS
|
|
118
|
------------------------------------------------------------------------------------------------
|
|
119
|
|
|
120
|
proposal-issue-major-functional-2 changes what this key actually does; land first, then write this
|
|
121
|
javadoc once (see 2.1)
|
|
122
|
proposal-issue-minor-functional-5 the same class of error in the WebClientProtocol javadoc; fix
|
|
123
|
both in one pass so the two descriptions agree
|
|
124
|
proposal-issue-major-functional-3 supplies the clamp referenced by 2.2
|
|
125
|
|
|
126
|
------------------------------------------------------------------------------------------------
|
|
127
|
4. RISK AND VERIFICATION
|
|
128
|
------------------------------------------------------------------------------------------------
|
|
129
|
|
|
130
|
Risk: none - comments only.
|
|
131
|
|
|
132
|
Verification
|
|
133
|
1. `./gradlew javadoc` produces no new warning (the {@link} target is in the same class).
|
|
134
|
2. Re-read the finished text against the three registration sites listed in section 1 and confirm each
|
|
135
|
claim is scoped: web client / other websockets / browser.
|
|
136
|
3. Sanity check the precedence claim by launching a spawned client and confirming the effective idle
|
|
137
|
timeout in the "Keep-alive ping started with interval=..., idle timeout=..." log line matches
|
|
138
|
client:web:socketTimeout, not client:web:maxIdleTime.
|