Repository navigation
Close a WebSocket client after a failed or partial frame write - #728
Merged
Merged
Conversation
`_webui_send_client_ws()` only logged the result of `mg_websocket_write()`. When a client stops reading for longer than `request_timeout_ms` (30 s), or the socket fails, CivetWeb returns after writing only part of a frame. The connection stayed open and the next frames followed the partial one, so the client read them out of step. A failed or short write now marks the connection as broken, so nothing more is written to it, and closes it with `mg_close_connection()`. The check, the write and the close run under the connection's own (recursive) CivetWeb lock, so no global lock is held during the write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AlbertShown
approved these changes
Oct 26, 2026
Member
|
Thank you @ViktorJannicke |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_webui_send_client_ws()stores the result ofmg_websocket_write()inretand only logs it (src/webui.c:10897on 2fb97ed). CivetWeb'smg_websocket_write_exec()writes the frame header and then the payload with twomg_write()calls (src/civetweb/civetweb.c:13950,:13962). It returns -1 if the header was not written completely, otherwise the result of the payload write, which can be fewer thanpackets_sizebytes or -1. A write ends early when the client stops reading for longer thanrequest_timeout_ms(default 30 s,push_all(),civetweb.c:6327; WebUI doesn't set it) or when the socket fails.Part of a frame is then on the wire, but the connection stays open and the next frames are written after it. The client reads them out of step: the rest of the cut frame's payload is taken from the following frames, so the bridge gets a corrupted script or message, and every frame after it is misread. Since #725 only that client is affected, but its pending calls are lost without a close event.
Several frames for the same client are often waiting at that point: other threads block in
mg_websocket_write()on the connection lock while the first write times out, and write their frames as soon as it returns.Fix
In
_webui_send_client_ws():packets_sizebytes, the connection is marked as broken in a new internal_webui.clients_broken[](next toclients[], undermutex_client) and closed withmg_close_connection()._webui_connection_save()clears the mark when a slot is (re)used, because CivetWeb reuses itsstruct mg_connectionfor later connections.The page then sees the connection close, and the bridge reconnects (
#wsStayAliveis true by default). WebUI's normal close path runs unchanged: CivetWeb calls the close handler,_webui_connection_remove()frees the slot, the window getsWEBUI_EVENT_DISCONNECTED, and waitingwebui_script()calls fail via_webui_script_client_closed().Successful writes behave as before. Nothing changes in
src/civetweb/orinclude/webui.h, and no exported function, type or value changes.Thread safety
mg_lock_connection(client). That is the lockmg_websocket_write()already takes for every frame (civetweb.hdocuments it for server-initiated websocket writes), and it is recursive (PTHREAD_MUTEX_RECURSIVE/ a critical section on Windows). A sender that was waiting for the connection therefore sees the broken mark and writes nothing. The lock is per connection, so as after Fix one stalled client blocking WebSocket writes to all windows #725 a stalled client blocks only writes to itself.mutex_clientis taken inside the connection lock only for the read and the update of the mark. No code holdsmutex_clientwhile it takes a connection lock (_webui_connection_remove()releases it beforemg_close_connection()), so this lock order can't deadlock.mg_close_connection()is the call WebUI already uses to close clients from other threads (webui_close_client(),_webui_connection_remove()). While the connection's thread is in its read loop it only setsmust_close(civetweb.c:18441). The loop checks that flag after each read, so CivetWeb's thread leaves the loop itself, calls the close handler and closes the socket. If the loop has already ended (the client closed at the same moment),mg_close_connection()closes the socket at once, under the connection lock, and CivetWeb's own close later finds it closed.webui_close_client()behaves the same way in that case.close_connection()before it can reuse the connection struct. The sender holds that lock from the write to the close, so the close always applies to the connection that got the cut frame.One limitation: the close takes effect when CivetWeb's read wait for that connection returns, which is when the client sends anything (the bridge sends a keep-alive at least every 20 s while the page runs) or after
websocket_timeout_ms(3600 s in WebUI). No frames are written to the connection in the meantime. CivetWeb has no public API for shutting down the socket of a connection that is in its read loop from another thread.Testing
Linux x64, gcc 16.2 (
make,make debug) and clang 21 (compile only): no new warnings (the only warning,system()unused result, is also on main).examples/C/minimalbuilds.Reproducer (not part of this PR): a C app with
multi_clientthat callswebui_run()with 1 MiB scripts from 2 threads, and a raw WebSocket client that does the bridge's token check, reads 3 frames, stops reading for 45 s (still sending the bridge'spingevery 5 s), then reads again and checks every frame.82 7f 00 00 00 00 00 10 00 09 dd ...at byte 751,536 of the script) and the following frames are out of step. 3 of 3 runs.WEBUI_EVENT_DISCONNECTEDin the app). The client gets the start of the cut frame and then the end of the connection (a reset, as its next keep-alive went to the closed socket). No further frames are written. 3 of 3 runs.master_thread_run()/mg_stop()). The fix adds one:mg_close_connection()writes the plainintmust_closeflag whileread_websocket()reads it (civetweb.c:18443/:13526). That is how CivetWeb closes a connection from another thread, andwebui_close_client()writes the same flag the same way. There are no reports in the new WebUI code.🤖 Generated with Claude Code