Hi Roger, Sorry for this large reply.
In short:
We need to decide whether the current patch suffices
for now or if a long-term overhaul of the
WebSocket implementation makes sense.
Where v3 improves upon `main`:
Based on the first 12 scenarios, `main` stands at 4 passed / 5 failed / 3
hanging,
whereas v3 stands at 12 / 0 / 0 on both platforms. The issue of a
hang during termination while a read operation is stuck has thus been resolved.
Where v3 performs worse than `main`:
13: Termination from a synchronized method – `main`: returns after 2–3 s
(abandons the thread) – v3: hangs
14: Close-frame callback disconnects an earlier client – `main`: does not
report B – v3: removes an unrelated, healthy connection (stale index)
15: Reset mid-frame, Windows: – `main`: never reported – v3: busy loop,
~126,000 OnError events/s
20: Client released while its OnDisconnect is running – `main`: indeterminate
(never reports) – v3: connection is released during the notification
In contrast, scenarios 17, 19, and the new scenarios 21–36 (hangs,
use-after-free) are legacy issues carried over from `main`.
Long version:
-------
- Your claims hold. Every scenario from the earlier rounds that failed
or hung on main passes with v3 (the two TLS scenarios measured on
linux only), and --pump-first no longer crashes.
- One scenario is worse than on main, scenario 13: Terminate called
from a method a callback synchronized onto the main thread. Main
returns (it gives the thread up); v3 never returns.
- Four more scenarios fail on both trees, but differently with v3:
a close-frame callback that disconnects an earlier client makes the
pump drop a third, healthy client (14); a client freed while its
OnDisconnect notification runs loses its connection under that
notification (20, and 29 with a reconnect); a client that disconnects
itself from its close-frame callback stops the pump from serving the
others (26). Main does not reach most of these paths, because it never
acts on a peer close.
- The test now has 36 scenarios. Most of the new ones fail or hang on
main already: disconnecting, reconnecting or freeing a client from its
own callbacks, from methods they synchronize, or while its read is
stalled. They are not caused by your patch; I list them because they
are in the same code and in the same test.
- On Windows, a connection reset in the middle of a frame is still never
reported with v3, and OnError fires without end (15). Linux reports
it.
Everything below was run on this machine, each scenario in its own
process with a 45 s watchdog and a 60 s external limit, so a hang is
reported instead of stalling the run.
- aarch64-win64: FPC 3.3.1, unmodified main 8604d19b11, complete
snapshot build including packages. v3 built from your patch applied to
the same revision. No OpenSSL on this machine, so the two TLS
scenarios are skipped.
- aarch64-linux (WSL Ubuntu 24.04, OpenSSL 3.0.13): FPC 3.3.1 whose
fcl-web, fcl-net and openssl sources are identical to main 8604d19b11
(the compiler itself carries unrelated local changes). v3 built from
the same patch.
Before measuring, each binary was checked for what it contains: the v3
builds carry EWSReadInterrupted, the main builds do not, and the build
logs show which units were loaded.
The numbers
-----------
Full runs, 36 scenarios:
linux main linux v3 win main win v3
passed 8 18 8 15
failed 19 11 17 12
skipped 0 0 2 2
hung 9 7 8 7
crashed 0 0 1 0
Per scenario (P passed, F failed, H hung, S skipped, C crashed):
4 columns: linux main, linux v3, win main, win v3
linux win
main v3 main v3
1 upgrade handshake and echo P P P P
2 repeated Execute/Terminate P P P P
3 peer close F P F P
4 Terminate while idle P P P P
5 TLS echo and peer close F P S S
6 partial frame stall, plain TCP H P H P
7 partial frame stall, TLS H P S S
8 Terminate from OnDisconnect F P F P
9 healthy client beside a stalled one H P H P
10 Terminate while a callback synchronizes P P P P
11 queued connection destroyed earlier F P F P
12 exception in a later client F P F P
13 Terminate from a synchronized method P H P H
14 close-frame callback removes a client F F C F
15 peer closes or resets mid-frame F P F F
16 OnError synchronizes a disconnect P P P P
17 OnMessage synchronizes a disconnect H H H H
18 client freed during its own callback P P P P
19 client freed, its callback synchronizes H H H H
20 client freed during its notification F F F F
21 synchronized method disconnects it H H H H
22 synchronized method reconnects it H H H H
23 OnDisconnect synchronizes a reconnect F P F P
24 OnDisconnect reconnects directly F P F P
25 client disconnects itself (message) F F F F
26 client disconnects itself (close frame) F F F F
27 stalled client freed, no Terminate H H H H
28 stalled client disconnected, no stop H H H H
29 reconnect during the notification F F F F
30 worker frees a synchronizing client P P P P
31 reconnected client freed, old callback F F F F
32 pump freed, old callback still running F F F F
33 reconnect inside its own OnDisconnect F F F F
34 pump reassigned during a callback F F F F
35 connection destructor raises F F F F
36 OnDisconnect raises during Free F F F F
Single runs - all 36 on main, 11-36 except 19 on v3 - give the same
sets, with two differences on Windows main: scenario 8 hangs when run
alone and fails inside the runner, as in the earlier rounds, and 14
fails alone instead of crashing. "no stop" in 27/28 means without
calling Terminate first. --interrupt-race passes on all four;
--pump-first passes with v3 and exits 217 on main.
Not every F is a failure. On main, 20, 23, 24 and 29 decide nothing:
main never reports a peer close, so the situation they stage does not
arise. On both trees, 31, 32 and 34 decide nothing either: they need the
owner to act while a callback is still running, and the owner's call
waits for that callback. The test says so in its output instead of
counting a pass.
Worse with v3
-------------
Scenario 13. An OnMessage callback synchronizes a method onto the main
thread, and that method calls Terminate - an application closing its
window when a message arrives. The reader waits for the method to
return; the method waits in Terminate for the reader. Main returns
because its loop is bounded and then abandons the thread; v3 joins. Same
on both platforms. Would it be possible for Terminate to stay bounded
when it is called while the reader waits for the calling thread?
Failing on both, but differently
--------------------------------
Scenario 14. Three clients; the pump receives a close frame for B, and
B's control callback disconnects the earlier client A. With v3 the pump
then removes the wrong entry: C, which nobody closed, is no longer
tracked by the pump, still reports Active=True, gets no OnDisconnect,
and its echo is never read. Main keeps C but does not report B at all.
Could the position be determined again after the callback, or the entry
removed by pointer?
Scenario 20. The peer closes; the pump notifies the owner, whose
OnDisconnect takes a while, and the owner frees the client meanwhile.
With v3 Free returns at once and the connection is destroyed while the
notification for it still runs (the test connection keeps its memory,
so this is counted rather than a crash). 29 shows the same with a
reconnect instead of Free. Could the connection stay alive until the
notification has returned?
Scenario 26. A client disconnects itself from the control callback for a
close frame. On both trees the connection is used after its destruction
(the close reply is sent on it); with v3 the pump afterwards also stops
serving the other client in the test.
Scenario 15 on Windows. With v3, an orderly close in the middle of a
frame is reported after about 1.2 s; a reset is never reported, the
connection stays Active and tracked, and OnError fired 911 584 times in
the measuring window, 107 312 of them in the last second (the last
message: "Close message already sent, cannot send more data."). What I
found when I first saw this is older than your patch:
TWSFramePayload.ReadData retries empty reads 100 x Sleep(1) and then
raises a plain Exception ("20230316102741 TWSFramePayload.ReadData"),
while the header read only checks for 0, and Windows returns -1 on every
recv after a reset. On linux v3 reports the close after 112 ms and the
reset after 108 ms. Is the Windows case worth a separate report?
Already on main
---------------
These fail or hang identically with and without v3. They are in the
same code, so I include them, but they are not about your patch.
- Hangs: 17 and 21/22 (a method synchronized by a callback disconnects
another client, or disconnects or reconnects the client whose callback
waits), 19 (a client freed while its callback waits in Synchronize),
27/28 (a client whose read is stalled is freed or disconnected without
calling Terminate first; the call never returns).
- Use after destruction: 25 and 26 (a client disconnects itself from its
own OnMessage or close-frame callback; HandleIncoming goes on, or the
close reply is sent, on the destroyed connection), 33 (OnDisconnect
reconnects inside that callback; the client ends up inactive, and the
new connection is not served).
- 36: OnDisconnect raises while the client is freed. Free raises the
handler's exception, and neither the client nor its connection is
destroyed.
- 35 checks a policy rather than a defect: a connection destructor that
raises reaches the thread that disconnects. Whether that is wrong is a
design question; I only report what happens.
Is disconnecting, reconnecting or freeing a client from its own
callbacks, or from methods they synchronize, meant to be supported? If
it is, these belong to the same area; if not, it may be worth saying so
in the unit.
The test
--------
The updated test file is attached. Scenarios 1-12 are the ones from the
earlier rounds with their numbering unchanged; 13-36 are new. The
lifetime checks use test connections and clients whose FreeInstance
keeps the memory, so a use after destruction is counted instead of
crashing.
Sven<<attachment: wsshutdowntest.zip>>
_______________________________________________ fpc-pascal maillist - [email protected] https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
