Hi Michael, Sven,

Short version: I have addressed all three issues Sven identified: waking a
blocked reader on Windows, preventing a reader thread from joining itself,
and avoiding interruption of healthy connections sharing the same pump.

On Windows, a blocked reader is stopped in this order:

   1.

   fpShutdown(SHUT_RDWR) prevents subsequent socket reads or retries.
   2.

   CancelIoEx(socket, nil) releases the recv operation already in progress.
   3.

   A persistent interruption marker unwinds the WebSocket frame parser
   instead of allowing another read attempt.
   4.

   The message pump waits for and joins the reader thread.
   5.

   Only then are the socket, transport, and TLS objects destroyed.

The essential safety rule is therefore preserved: no networking or OpenSSL
state is freed while the reader thread may still be using it.

Only fpwebsocket.pp and fpwebsocketclient.pp are changed. The server and
upgrader units remain unchanged. The actual HTTP-to-WebSocket upgrade tests
pass over plain TCP and TLS.

Changes to the original code

The original TWSThreadMessagePump did not reliably control the lifetime of
its reader thread.

The worker used FreeOnTerminate := True. Terminate requested termination
and then polled the thread reference for approximately two seconds. If the
worker remained blocked, Terminate cleared the reference without proving
that the thread had stopped.

The connection, transport, socket handler, and OpenSSL objects could
consequently be destroyed while the abandoned reader was still inside
SSL_read. This explains the observed SIGSEGV during reconnection or
shutdown.

ReadConnections also discarded the result of CheckIncoming. An irClose
result therefore did not unregister the connection, clear Active, or fire
OnDisconnect.

In addition, the main connection-list lock was held while reading a
complete WebSocket frame. A peer sending only part of a frame could leave
the reader blocked while holding that lock, preventing ordinary removal or
shutdown.

The stale connection state and the OpenSSL crash were two consequences of
the same incomplete reader-thread lifecycle.

What the revised patch does

The message pump now owns a named TMessageDriverThread. It creates the
thread suspended with FreeOnTerminate := False, publishes the reference,
and then starts it. During external termination, the pump waits for the
actual thread to finish, calls WaitFor, and frees it only after it has
stopped.

A healthy pump is still stopped normally. Terminate first requests
termination and allows the bounded polling loop to exit without disturbing
its connections. Repeated Execute and Terminate cycles therefore continue
to work.

If the reader is genuinely blocked, the fallback interrupts only the
transport currently inside a low-level read. A small atomic read state
identifies and claims that transport. This prevents a stalled connection
from causing healthy connections on the same pump to be interrupted.

No critical section has been added to normal message processing. The read
path uses interlocked state changes. The separate locked transport registry
is used only during registration, removal, and termination.

Why CancelIoEx is used on Windows

An isolated raw-socket test produced the following results over three
rounds:

CancelIoEx(socket, nil):
Released 3 of 3 blocked fpRecv calls immediately.
fpRecv returned -1 with error 10004, WSAEINTR.

QueueUserAPC:
Released 0 of 3 blocked reads, although every APC callback executed.

CancelSynchronousIo:
Released 0 of 3 blocked reads and returned error 1168, ERROR_NOT_FOUND.

fpShutdown(SHUT_RDWR):
Accepted the call but released 0 of 3 receives already in progress.

CancelIoEx is therefore used to cancel the receive already in progress,
while fpShutdown prevents the reader from entering another receive
afterward.

CancelIoEx is resolved dynamically, so the patch does not introduce a new
loader dependency on that entry point on older Windows systems.

A cancelled read cannot block again

The socket helper retains its interruption request until the complete read
stack has unwound. It raises EWSReadInterrupted instead of allowing the
frame parser or OpenSSL handler to treat the cancellation as a short read
and retry.

A partially consumed WebSocket frame cannot safely be resumed, so the
affected connection is removed and its owner is notified.

Peer close and callback handling

An irClose result now removes the connection from both pump registries,
clears the client's active state through the existing disconnection path,
and fires OnDisconnect.

Callbacks run after the main pump-list lock has been released.

Because OnDisconnect executes on the reader thread, a handler may call
Terminate from that same thread. In this case, Terminate requests
termination but does not attempt to join or free its own thread. A later
call by the owner or destructor completes the join and cleanup.

Regression-test results

HTTP-to-WebSocket upgrade and echo: passed
Repeated Execute/Terminate, five rounds: passed
Peer close, Active=False, exactly one OnDisconnect: passed
Termination with an idle healthy connection: passed
TLS upgrade, echo, and peer close: passed
Partial-frame stall over plain TCP: passed; Terminate returned in
approximately 125 ms
Partial-frame stall inside TLS: passed; Terminate returned in approximately
125 ms
Terminate called from OnDisconnect: returned normally
Healthy and stalled clients sharing one pump: healthy client remained
active and usable

The attached wsshutdowntest.pas is a command-line test for the patched
WebSocket units.

On macOS, FPC targets Darwin and uses the Unix fpShutdown branch; the
Windows-only CancelIoEx code is excluded. Apple documents that shutdown
disables further reads, but does not explicitly guarantee that it releases
a receive already in progress. A run of wsshutdowntest.pas on Apple Silicon
would settle this. Scenarios 7 and 8 are the decisive blocked-read tests
over plain TCP and TLS.

Please test and verify these results.

Regards,

Roger

Attachment: fpc-websocket-safe-shutdown-cancelioex-v2.patch
Description: Binary data

Attachment: wsshutdowntest.pas
Description: Binary data

_______________________________________________
fpc-pascal maillist  -  [email protected]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal

Reply via email to