|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - WebClientProtocol.sendLock
|
|
3
|
Field javadoc still documents the partial-frame contract the rewrite removed
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"The field javadoc still documents the removed contract [...] Every clause after 'Both' is now
|
|
8
|
false: after the MSG_CHUNK rewrite there are no partial frames, and sendBinaryMessageStreamed
|
|
9
|
does not acquire sendLock [...] PushMessagesWorker's copy of this javadoc was updated by the same
|
|
10
|
diff; this one was missed."
|
|
11
|
|
|
12
|
FILES
|
|
13
|
src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java
|
|
14
|
|
|
15
|
------------------------------------------------------------------------------------------------
|
|
16
|
1. ROOT CAUSE
|
|
17
|
------------------------------------------------------------------------------------------------
|
|
18
|
|
|
19
|
Current text, WebClientProtocol.java:385-391:
|
|
20
|
|
|
21
|
/**
|
|
22
|
* Gate serializing every actual websocket send. Both the {@link PushMessagesWorker}'s
|
|
23
|
* whole-message sends and {@link #sendBinaryMessageStreamed} (which emits many partial frames for
|
|
24
|
* one logical message) acquire this, so a streamed frame sequence is never interleaved with
|
|
25
|
* another send on the same socket (which would corrupt the message and can raise
|
|
26
|
* {@code WritePendingException}).
|
|
27
|
*/
|
|
28
|
private final Object sendLock = new Object();
|
|
29
|
|
|
30
|
Three claims, all now false:
|
|
31
|
|
|
32
|
- sendBinaryMessageStreamed does not acquire sendLock. It queues whole messages via queueChunk
|
|
33
|
(:1003), which takes lock, not sendLock.
|
|
34
|
- There are no partial frames. sendPartial was deleted; the rewrite sends ordinary messages.
|
|
35
|
- There is no "streamed frame sequence" to protect from interleaving. Interleaving is now the
|
|
36
|
explicit goal - sendBinaryMessageStreamed's own javadoc (:929-931) says the heartbeat and other
|
|
37
|
output "interleave freely between pieces", i.e. the exact opposite of what the field doc
|
|
38
|
promises.
|
|
39
|
|
|
40
|
sendLock occurs at three places in the file: this declaration, sendBinaryMessageSync (:1090), and
|
|
41
|
the PushMessagesWorker constructor argument (:1966). Those two are the real holders.
|
|
42
|
|
|
43
|
The same javadoc on PushMessagesWorker.sendLock (:138) was rewritten by this diff to the accurate
|
|
44
|
wording; only this copy was missed. That asymmetry is what makes it worth fixing rather than
|
|
45
|
tolerating - two fields named sendLock now document contradictory contracts.
|
|
46
|
|
|
47
|
------------------------------------------------------------------------------------------------
|
|
48
|
2. PROPOSED FIX
|
|
49
|
------------------------------------------------------------------------------------------------
|
|
50
|
|
|
51
|
Replace with a description of the contract that actually holds:
|
|
52
|
|
|
53
|
/**
|
|
54
|
* Gate serializing every actual websocket send. Held by {@link PushMessagesWorker} around each
|
|
55
|
* whole-message send and by {@link #sendBinaryMessageSync}, so two sends never overlap on the
|
|
56
|
* same socket (which would corrupt the message and can raise {@code WritePendingException}).
|
|
57
|
* <p>
|
|
58
|
* It is only ever held across a single whole message. Large payloads travel as a series of
|
|
59
|
* ordinary {@link #MSG_CHUNK} messages queued by {@link #sendBinaryMessageStreamed}, which does
|
|
60
|
* not take this lock, so nothing occupies the socket for a whole transfer and the keep-alive
|
|
61
|
* heartbeat can always get a frame out between pieces.
|
|
62
|
*/
|
|
63
|
private final Object sendLock = new Object();
|
|
64
|
|
|
65
|
This mirrors the wording already applied to PushMessagesWorker.sendLock, so the two fields now read
|
|
66
|
consistently, and it states the non-obvious part - that streamed sends deliberately do *not* hold
|
|
67
|
it - which is the fact a maintainer is most likely to get wrong.
|
|
68
|
|
|
69
|
Leave the file history block alone. Entry 045 already covers the MSG_CHUNK rewrite in this
|
|
70
|
revision, and if this correction lands in the same commit it needs no separate entry; the historical
|
|
71
|
PushMessagesWorker entry 009 ("Serialize all sends on a shared sendLock (so streamed partial-frame
|
|
72
|
sends cannot interleave)") is a record of what was done then and is correctly left untouched.
|
|
73
|
|
|
74
|
------------------------------------------------------------------------------------------------
|
|
75
|
3. RELATIONSHIP TO OTHER PROPOSALS
|
|
76
|
------------------------------------------------------------------------------------------------
|
|
77
|
|
|
78
|
None functionally. If proposal-issue-minor-performance-1 is taken, it changes how
|
|
79
|
sendBinaryMessageStreamed throttles but not whether it takes sendLock, so this wording stays
|
|
80
|
correct either way.
|
|
81
|
|
|
82
|
------------------------------------------------------------------------------------------------
|
|
83
|
4. RISK AND VERIFICATION
|
|
84
|
------------------------------------------------------------------------------------------------
|
|
85
|
|
|
86
|
Risk: none. Comment-only.
|
|
87
|
|
|
88
|
Verification
|
|
89
|
1. ./gradlew compile - the {@link #MSG_CHUNK} and {@link #sendBinaryMessageSync} references must
|
|
90
|
resolve (MSG_CHUNK is inherited from WebClientMessageTypes, so #MSG_CHUNK is valid here; use
|
|
91
|
{@link WebClientMessageTypes#MSG_CHUNK} if javadoc warns).
|
|
92
|
2. ./gradlew javadoc produces no new warnings for this file.
|
|
93
|
3. Read the two sendLock javadocs side by side and confirm they now describe the same contract.
|