Hi,
I maintain the websocket stuff.
Did you test whether the upgrade path still works after your changes ?
Michael.
On Sun, 6 Sep 2026, Roger Olsson via fpc-pascal wrote:
Okay, here we go:I have debugged and made a patch based on todays trunk
(attached) for a fpwebsocketclient recovery/shutdown problem on Linux with
OpenSSL. After a network interruption, the client could remain active or
“reconnecting” while receiving no further data until the application is
restarted. During reconnect/shutdown I also saw a SIGSEGV with the reader
thread inside SSL_read.
Cause: fpwebsocketclient did not always stop and join its reader thread
before the connection and OpenSSL objects are destroyed. It also failed to
act on irClose. The result could be either a stale connection that never
received data again after a network interruption, or a SIGSEGV when
transport cleanup raced with SSL_read.
The best solution was to fix this inside FPC: make the message pump own the
reader thread, wake a genuinely stuck socket when necessary, join the
reader, and only then destroy the connection and TLS objects. I have
modified my local files and done successful runtimes tests. The rest of the
fpc websocket code should be unaffected.
Proposed changes
• The pump should own a named, non-self-freeing thread using
FreeOnTerminate := False.
• Execute, Terminate, and destruction should be idempotent.
• Shutdown should follow this order:
1. Request thread termination.
2. Allow the bounded polling loop a short opportunity to exit
normally.
3. If it remains blocked, wake the socket with fpShutdown(...,
SHUT_RDWR).
4. Join and free the reader thread.
5. Only afterward close and free the connection, transport, and TLS
objects.
The wake operation must be raw socket shutdown only. Calling the TLS
handler’s Shutdown, SSL_shutdown, or freeing OpenSSL state while another
thread is inside SSL_read recreates the race.
ReadConnections should also process irClose: unregister the connection,
mark the client inactive, and notify its owner, preferably after releasing
the pump-list lock.
Because the reader currently holds the main connection-list lock while
reading, termination needs a separate cold-path transport registry or
another safe snapshot. This requires no additional per-message or hot-path
locking.
For compatibility, socket interruption should only be the fallback for a
reader that did not stop normally. Healthy connections should not be
unconditionally shut down merely because the pump was stopped.
More in depth, the problems appear to be:
TWSThreadMessagePump.Terminate can return without proving that the worker
has stopped. The timeout path clears the thread reference, allowing the
connection, transport, or TLS objects to be freed while the worker may
still be inside CheckIncoming/SSL_read.
ReadConnections ignores the irClose result, so a peer-closed connection can
remain registered and appear active.
WaitFor alone is not sufficient: select is bounded, but after the first
bytes of a frame arrive, reading the remainder of a partial WebSocket frame
or TLS record can block indefinitely.
Suggested changes i detail:
Make the pump own its thread: create it suspended, set FreeOnTerminate :=
False, publish the reference, then start it.
Make Execute, Terminate, and destruction idempotent.
During termination, first request termination and allow a short bounded
interval for the normal polling loop to exit.
If the reader is still running, wake it with raw fpShutdown(Socket.Handle,
SHUT_RDWR), then call WaitFor and free the thread.
Do not call TSocketHandler.Shutdown, SSL_shutdown, close the descriptor, or
free transport/TLS objects before the reader has joined. The
OpenSSL socket handler’s shutdown path destroys SSL state and is unsafe
concurrently with SSL_read.
Keep a separate cold-path registry or another safe snapshot of registered
transports, because ReadConnections currently holds the main list lock
while reading. The termination path must not wait for that same lock before
it can wake the blocked reader.
Handle irClose by safely unregistering the connection, marking the client
inactive, and notifying its owner—preferably after releasing the pump-list
lock.
Preserve normal client-processing order and existing protected signatures
where practical. Raw socket interruption should be the stuck-reader
fallback rather than unconditional behavior, so applications that
temporarily stop and restart a healthy pump retain compatibility.
Useful regression tests would cover partial-frame/TLS-record stalls,
repeated Execute/Terminate, destruction during a blocked read, peer close
and OnDisconnect, and multiple clients sharing one pump.
/Roger
_______________________________________________
fpc-pascal maillist - [email protected]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
