Hi Roger, I had the analysis carried out.
Summary
-------
- All three points from the first round are fixed, on both platforms.
Windows is now as good as linux: Terminate on a reader blocked in a
socket read returns in 110-141 ms, over plain TCP and inside TLS.
- One new hang, scenario 10: a callback parked in TThread.Synchronize
while the main thread is in Terminate. Nobody services it.
- Two new observations in ReadConnections, scenarios 11 and 12,
measured this time rather than argued. A connection collected for
notification and destroyed by an earlier OnDisconnect callback is
still used afterwards; an ordinary exception from a later client
drops a notification that was already pending. Both end in a
question below rather than in a verdict.
- The EndRead window I asked about last time is not hit by your patch.
It took a deliberately slowed copy to see any consequence at all, so
I can report what happens if it is hit, not how often it is.
- Your patch also fixes something your description does not mention:
TWSMessagePump.Create never called inherited Create(aOwner).
- Unchanged, as you say: --pump-first still crashes.
Everything below was run on this machine. Measured against main
8604d19b11 - both blob hashes in your patch header match it and
git apply --check passes. Two complete trees differing only in the
patch, aarch64-win64 (FPC 3.3.1-20832) and aarch64-linux (WSL Ubuntu
24.04, OpenSSL 3.0.13). Each scenario runs in its own process with a
60 s external timeout, so a hang is reported rather than stalling the
run.
The test is my own file back again: from "program wsshutdowntest;"
onwards your copy is byte-identical to mine, so your numbers and mine
come from the same code. I took your corrected header - mine still
listed the old scenario numbering.
The numbers
-----------
linux without linux with win without win with
passed 4 9 4 9
failed 5 2 5 2
hung 3 1 3 1
Twelve scenarios. The two failures on the patched side are the new
scenarios 11 and 12, the hang there is scenario 10. Both platforms
produce the same sets, not merely the same counts.
One caveat on the left-hand columns: on the unpatched tree the split
between failed and hung is not stable. Scenario 8 hit the watchdog in
three standalone runs on Windows but reported its checks inside the
runner, so the same binary gives 5 failed / 3 hung or 4 failed / 4 hung
depending on the run. The patched columns were stable across every run
I made.
What is fixed:
- Windows now wakes. Scenarios 6 and 7, the partial-frame stall over
plain TCP and inside TLS, hung with the first version. Terminate now
returns in 110-141 ms on Windows and 101-104 ms on linux, three runs
per platform, no outliers.
- No self-join: scenario 8, Terminate from the OnDisconnect callback,
passes.
- No collateral damage: scenario 9, a healthy client sharing the pump
with a stalled one, passes.
- irClose is acted on, over TLS as well.
The one that still hangs, and it is new
---------------------------------------
I added a tenth scenario after the review, because the question was
measurable. An OnMessage callback calls TThread.Synchronize. That parks
the reader thread until someone runs CheckSynchronize on the main
thread. The main thread is in Terminate.
without the patch: Terminate returned after 3344 ms -> passes
with the patch: never returns; the in-process watchdog reports at
45 s and the runner kills the child at 60 s
Same on both platforms. Interrupting sockets cannot help here: the
reader is not in a read, it is waiting for the main thread.
To be fair about what changed: the old loop does not service
CheckSynchronize either. It is merely bounded - 200 x 10 ms, then it
gives up, sets FThread := Nil and leaves the thread running, which is
the lifetime bug you are fixing. So the outcome moves from "a thread
quietly kept running" to "the call never returns". Both are wrong; one
of them is visible. TThread.WaitFor does service CheckSynchronize on
both platforms (rtl/win/tthread.inc, rtl/unix/tthread.inc), but the new
polling loop before it is never left. Would it be possible for the wait
to service synchronization, or for the fallback to stay bounded?
Something the patch fixes that you do not mention
-------------------------------------------------
TWSMessagePump.Create(aOwner) in main never calls inherited
Create(aOwner). Measured with a program that only constructs and
destroys, no sockets, no threads: without the patch Owner is nil and
the owner's ComponentCount stays 0; with it, both are correct.
Worth naming in the commit message, because it changes behaviour - code
that passed an owner had to free the pump itself and now the owner
frees it too. Freeing it before the owner stays correct, since
TComponent.Destroy unregisters; a pointer kept past the owner's
destruction does not.
Two more in ReadConnections, measured
-------------------------------------
Last time I listed these as things I had only read. They are scenarios
now, and both reproduce on Windows and linux alike, three runs each.
Scenario 11 - a queued connection destroyed by an earlier callback.
Closed connections are removed from both registries under the list
lock, collected, and disconnected after the unlock. Between the first
and the second of those calls the application's OnDisconnect runs, and
tearing the remaining connections down from there is an ordinary thing
for an application to do.
Two peers close before the pump is started, so its first pass collects
both in the same pass. The test asserts that rather than assuming it:
before the pump runs, both connections must report irWaiting from
CheckIncoming(50,False), otherwise the round is declared undecided
instead of clean. The first callback then calls Disconnect(False) on
the second client, which frees its connection object, and the pump goes
on to call Disconnect on that pointer.
control round, no teardown:
OnDisconnect A=1 B=1, calls after destruction=0
provocation:
OnDisconnect A=1 B=1, calls after destruction=1
So that this is an observation rather than a crash, the test connection
overrides FreeInstance: the destructor and CleanupInstance run as
usual, only the FreeMem is skipped, so the call lands countably in the
class instead of in whatever the heap manager has put there. In an
ordinary build that block is back on the free list.
Could a collected connection stay protected until its callback has run?
Scenario 12 - an exception in a later client. Both loops sit inside one
try..except, so an ordinary exception from a later client jumps past
the notification loop. Client A's peer closes; client B's OnMessage
raises a plain Exception. Same readiness assertion as above: both must
be waiting before the pump starts, or the round counts as undecided -
otherwise A could be notified in one pass and B raise in another, and
every check would hold without the arrangement ever existing.
control round, no failing callback:
errors=0, OnDisconnect A=1, A still tracked=False
provocation:
errors=1, OnDisconnect A=0, A still tracked=False,
B still tracked=True
A is in neither state: its owner was not told, and no later pump pass
can reach it, because it is in neither registry and the pending list
dies with the pass. It is of course still reachable through its owning
client - what is lost is the pump's own path to it. OnError fires
exactly once with the callback's own message, so the exception does
take the expected route, and 1.5 s of further passes produce no
notification.
Could the pending notifications be drained on that path?
Both scenarios also fail on main, and it is worth saying why, because
it is not the same reason: ReadConnections there discards what
CheckIncoming returns, so a peer close is never acted on at all.
Scenario 11 therefore never even reaches its provocation on main - the
test reports that round as undecided rather than as a pass. In scenario
12 main likewise does not notify, but it does leave the connection in
the list, so a later pass could still find it. Both scenarios are about
code your patch introduces; neither is a regression against something
that worked before.
On the locks I convinced myself there is no inversion: TThreadList.Add
and Remove take and release their own lock, and InterruptConnections
never touches FList. I had suspected one and was wrong.
The third one, and what it took to see it
-----------------------------------------
The EndRead window is a few instructions wide, so it is not a scenario
in the numbered run but a separate --interrupt-race mode.
A correction to what I wrote last time, and to my own first reading of
it: BeginRead's two checks catch a request that has already been
published, which is the common case, but they do not guarantee
publication before the next read - the interrupter can still be between
its CAS and its InterlockedExchange. So I am not claiming to know the
full set of affected readers.
What the mode stages is the case I could make deterministic: a reader
that does not read again in that pass, because the frame is complete.
The peer withholds half a frame so the reader parks in the payload
read; Terminate waits gracefully for 100 ms before it starts
interrupting, so the peer is armed from its own thread to deliver the
missing five bytes 120 ms in - about 20 ms into the interrupt loop.
Against your patch the window is not hit, on either platform:
Active=False, still tracked=False, OnDisconnect=1
To find out what happens if it is hit, the same mode ran against a copy
of your patch with a Sleep(50) between the CAS in InterruptRead and the
InterlockedExchange that publishes the request. That is a measuring
aid, not a suggestion:
Active=True, still tracked=True, OnDisconnect=0
a send afterwards: socket has been shut down / broken pipe
The connection stays registered and reports Active with a socket that
has been shut down, and nobody is told. That is the consequence; how
often the window is actually hit, this says nothing about.
Would it be worth having EndRead observe the request before it returns
normally?
Unchanged, as you say
---------------------
--pump-first still crashes (exit 217 on both platforms). That is the
missing Notification override, a pre-existing defect your patch does
not claim to address.
The updated test file is attached: scenarios 10, 11 and 12 and the
--interrupt-race mode are new, the rest is unchanged.
Sven
wsshutdowntest.pas
Description: Binary data
_______________________________________________ fpc-pascal maillist - [email protected] https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
