|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - WebClientProtocol.startKeepAlivePing
|
|
3
|
MIN_PING_INTERVAL floor is applied after the idle-timeout clamp, so it overrides it
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"interval = Math.max(interval, MIN_PING_INTERVAL) is applied *after* the idle-timeout clamp, so for
|
|
8
|
any idle timeout below PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL (3000 ms) the floor silently
|
|
9
|
overrides it and the invariant the javadoc asserts ('where an idle timeout is in force it is also
|
|
10
|
held to one PING_INTERVAL_DIVISORth of it') does not hold: idleMs=2000 gives 1000, i.e. half the
|
|
11
|
idle timeout rather than a third [...] and idleMs=1000 gives interval == idleMs, so since the ping
|
|
12
|
is only queued to PushMessagesWorker rather than written on the timer thread it cannot reach the
|
|
13
|
wire before Jetty's deadline - the session is reaped on the first quiet second, producing a
|
|
14
|
reconnect loop that the correctly-ordered clamp (333 ms) would have avoided. [...] Clamp in the
|
|
15
|
other order, i.e. floor the configured cadence and let the idle-derived bound win."
|
|
16
|
|
|
17
|
FILE
|
|
18
|
src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java
|
|
19
|
|
|
20
|
------------------------------------------------------------------------------------------------
|
|
21
|
1. ROOT CAUSE
|
|
22
|
------------------------------------------------------------------------------------------------
|
|
23
|
|
|
24
|
WebClientProtocol.java:1658-1661:
|
|
25
|
|
|
26
|
long interval = (idleMs > 0) ? Math.min(pingPongInterval, idleMs / PING_INTERVAL_DIVISOR)
|
|
27
|
: pingPongInterval;
|
|
28
|
|
|
29
|
interval = Math.max(interval, MIN_PING_INTERVAL);
|
|
30
|
|
|
31
|
Two clamps with opposite intent are applied in the wrong order. The floor exists to stop an absurdly
|
|
32
|
small *configured cadence* from flooding the socket; the ceiling exists to guarantee a *margin* below
|
|
33
|
the idle timeout. Applying the floor last lets it undo the ceiling:
|
|
34
|
|
|
35
|
idleMs ceiling (idleMs/3) result effective divisor
|
|
36
|
------- ------------------- ------- -----------------
|
|
37
|
3000 1000 1000 3 (boundary, correct)
|
|
38
|
2000 666 1000 2 (margin halved)
|
|
39
|
1000 333 1000 1 (no margin at all)
|
|
40
|
|
|
41
|
At idleMs == 1000 the ping is scheduled exactly at the idle deadline, and since it is only *queued* to
|
|
42
|
PushMessagesWorker rather than written on the timer thread, it cannot reach the wire in time - Jetty
|
|
43
|
reaps the session on the first quiet second and the client enters a reconnect loop. The correctly
|
|
44
|
ordered clamp yields 333 ms and avoids it.
|
|
45
|
|
|
46
|
Reachability: client/web/socketTimeout and client/web/maxIdleTime are plain Integers with no lower
|
|
47
|
bound (ConfigItem.java:374-375, :363-364) and dir_schema.xml has no client/web object class, so a
|
|
48
|
value of 1000 or 2000 is accepted verbatim. Low values are plausible in test and diagnostic setups.
|
|
49
|
|
|
50
|
------------------------------------------------------------------------------------------------
|
|
51
|
2. PROPOSED FIX
|
|
52
|
------------------------------------------------------------------------------------------------
|
|
53
|
|
|
54
|
Floor the configured cadence first, then let the idle-derived bound win:
|
|
55
|
|
|
56
|
- // The configured cadence is the baseline. When an idle timeout is in force the interval is also
|
|
57
|
- // held below it, since a ping landing at or after that deadline would be useless. When there is
|
|
58
|
- // no idle timeout the clamp is simply skipped - the heartbeat is not made conditional on one,
|
|
59
|
- // because without it Jetty never reaps the socket either and this check becomes the only dead
|
|
60
|
- // peer detection there is.
|
|
61
|
- long interval = (idleMs > 0) ? Math.min(pingPongInterval, idleMs / PING_INTERVAL_DIVISOR)
|
|
62
|
- : pingPongInterval;
|
|
63
|
-
|
|
64
|
- interval = Math.max(interval, MIN_PING_INTERVAL);
|
|
65
|
+ // The configured cadence is the baseline, floored so that a tiny configured value cannot flood
|
|
66
|
+ // the socket. The idle-derived bound is applied last and wins: it is a correctness bound, not a
|
|
67
|
+ // preference - a ping landing at or after the idle deadline can never reach the wire in time,
|
|
68
|
+ // since it is queued to the push worker rather than written on this thread. Applying the floor
|
|
69
|
+ // last would let it override that bound for any idle timeout below
|
|
70
|
+ // PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL. When there is no idle timeout the bound is simply
|
|
71
|
+ // skipped - the heartbeat is not made conditional on one, because without it Jetty never reaps
|
|
72
|
+ // the socket either and this check becomes the only dead peer detection there is.
|
|
73
|
+ long interval = Math.max(pingPongInterval, MIN_PING_INTERVAL);
|
|
74
|
+
|
|
75
|
+ if (idleMs > 0)
|
|
76
|
+ {
|
|
77
|
+ interval = Math.min(interval, Math.max(1L, idleMs / PING_INTERVAL_DIVISOR));
|
|
78
|
+ }
|
|
79
|
|
|
80
|
The Math.max(1L, ...) guards the degenerate idleMs < PING_INTERVAL_DIVISOR case: Timer.schedule throws
|
|
81
|
IllegalArgumentException on a non-positive period, and idleMs == 1 or 2 would otherwise produce 0. The
|
|
82
|
resulting interval is nonsensical at that point, but the session's own idle timeout is nonsensical too,
|
|
83
|
and a 1 ms ping is preferable to an uncaught exception on the connect path (startKeepAlivePing is
|
|
84
|
called from onConnect under the instance lock, so a throw there would abort session setup).
|
|
85
|
|
|
86
|
Resulting values:
|
|
87
|
|
|
88
|
idleMs pingPongInterval before after
|
|
89
|
------- ----------------- ------- -----
|
|
90
|
1000 30000 1000 333
|
|
91
|
2000 30000 1000 666
|
|
92
|
3000 30000 1000 1000 (unchanged)
|
|
93
|
90000 30000 30000 30000 (unchanged)
|
|
94
|
0 500 1000 1000 (unchanged: floor still applies)
|
|
95
|
|
|
96
|
Only idle timeouts below 3000 ms change.
|
|
97
|
|
|
98
|
------------------------------------------------------------------------------------------------
|
|
99
|
3. ALSO WORTH DOING (optional, adjacent)
|
|
100
|
------------------------------------------------------------------------------------------------
|
|
101
|
|
|
102
|
The finding notes the values are entirely unvalidated. Two cheap, separable improvements:
|
|
103
|
|
|
104
|
(a) Log a warning when the idle-derived bound actually overrides the configured cadence, so a
|
|
105
|
surprised administrator has a breadcrumb:
|
|
106
|
|
|
107
|
if (interval < pingPongInterval)
|
|
108
|
{
|
|
109
|
LOG.warning(String.format(
|
|
110
|
"The configured heartbeat interval %d ms exceeds the bound derived from the web " +
|
|
111
|
"socket idle timeout %d ms; using %d ms instead.",
|
|
112
|
pingPongInterval, idleMs, interval));
|
|
113
|
}
|
|
114
|
|
|
115
|
(b) Reject an idle timeout that cannot support a heartbeat at all (below, say,
|
|
116
|
PING_INTERVAL_DIVISOR * MIN_PING_INTERVAL) at configuration-read time in GuiWebDriver /
|
|
117
|
ChuiWebSimulator, alongside the pingPongInterval clamp those methods already do (see
|
|
118
|
proposal-issue-major-functional-3, which centralises that clamp). Out of scope for this
|
|
119
|
finding; note it rather than growing this change.
|
|
120
|
|
|
121
|
------------------------------------------------------------------------------------------------
|
|
122
|
4. RELATIONSHIP TO OTHER PROPOSALS
|
|
123
|
------------------------------------------------------------------------------------------------
|
|
124
|
|
|
125
|
proposal-issue-major-functional-2 the deadline is computed from this interval
|
|
126
|
(dataIdleMs = max(idleMs, interval * DATA_QUIET_MULTIPLIER));
|
|
127
|
with idleMs=1000 the deadline becomes max(1000, 999) = 1000 -
|
|
128
|
correct, and the ping now has a 2/3 margin inside it
|
|
129
|
proposal-issue-minor-functional-5 the javadoc sentence asserting this invariant is in the same
|
|
130
|
block and must be re-checked once the order is fixed
|
|
131
|
(after this change the assertion becomes true as written)
|
|
132
|
|
|
133
|
------------------------------------------------------------------------------------------------
|
|
134
|
5. RISK AND VERIFICATION
|
|
135
|
------------------------------------------------------------------------------------------------
|
|
136
|
|
|
137
|
Risk: none for any configuration with an idle timeout >= 3000 ms, which is every configuration shipped
|
|
138
|
in this workspace and the 90000 ms default.
|
|
139
|
|
|
140
|
Verification
|
|
141
|
1. Table-test the interval computation. This is the strongest argument for extracting it into a
|
|
142
|
package-private static helper (see proposal-issue-major-functional-2 section 4), since the whole
|
|
143
|
defect is arithmetic and there is currently no test for it. Cases: idleMs in
|
|
144
|
{0, 1, 2, 999, 1000, 2000, 3000, 90000, 600000} x pingPongInterval in {1, 1000, 30000}.
|
|
145
|
2. Run the Web GUI with client/web/socketTimeout=1000: the startup log must report interval=333 and
|
|
146
|
the session must stay up (before: interval=1000, reconnect loop).
|
|
147
|
3. Confirm no IllegalArgumentException from Timer.schedule with socketTimeout=1 and =2.
|