Repository navigation
Fix radio control self-deadlock on rigctld connection errors - #431
Merged
Merged
Conversation
When the rigctld connection error count reaches MAX_ERROR_COUNT,
rigctl_run() (the background worker thread) called
gtk_toggle_button_set_active() on the Engage button directly. That
call synchronously re-enters the same thread via rig_engaged_cb's
disengage branch, which does:
g_mutex_lock(&ctrl->widgetsync);
setconfig(ctrl);
g_cond_wait(&ctrl->widgetready, &ctrl->widgetsync);
g_mutex_unlock(&ctrl->widgetsync);
The only code that ever signals widgetready is rigctl_run's own
'else' branch further up in the same function -- code that cannot
run until this call returns. The result is a permanent self-deadlock:
the thread parks forever, and because rigctrl_close() (which resets
ctrl->sock to 0) lives in that same unreachable branch, it never
runs either.
This has two observable consequences, both confirmed in a headless
sandbox reproduction (Xvfb + a minimal fake rigctld-protocol TCP
server, killed and restarted to simulate a GRC flowgraph restarting):
1. Every subsequent Engage attempt reuses the dead, non-zero
ctrl->sock instead of reopening a connection, since rigctl_run's
own check (if (!t_ctrl->sock) rigctrl_open(t_ctrl);) sees a
stale-but-nonzero value and skips reconnecting entirely. Verified
directly: after simulating a flowgraph restart, the new listening
server received zero connection attempts, while Gpredict kept
logging errors against the old, closed socket.
2. gtk_toggle_button_set_active() and gtk_widget_set_sensitive() were
being called directly from a non-main thread, which is undefined
behavior in GTK3 (not thread-safe) and a plausible source of the
intermittent full-application freezes reported alongside this,
independent of the deadlock itself.
Fix: perform the actual cleanup (engaged flag, error count reset,
socket close, timer removal) directly on the worker thread, where it
belongs, then marshal only the GTK widget updates onto the main
thread via g_idle_add(). The toggled signal is blocked while updating
the button so rig_engaged_cb's own disengage branch -- meant for
user-initiated Engage/Disengage on the main thread -- does not run a
second time. The worker thread then breaks out of its loop and exits
cleanly, exactly as the normal (user-initiated) disengage path does.
Verified in sandbox: after this fix, MAX_ERROR_COUNT correctly logs
and disengages (previously unreachable), and a subsequent Engage
successfully opens a fresh connection to a restarted server, with a
full handshake and normal command cycling.
Fixes csete#425
Owner
|
It's merged, thanks! |
Owner
|
By the way, I pushed a change which eliminates the function-pointer cast warning - please let me know if it breaks it :) |
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
2 participants
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.
Fixes #425.
When the rigctld error count reaches MAX_ERROR_COUNT, rigctl_run()
called gtk_toggle_button_set_active() from the worker thread. That
re-entered rig_engaged_cb() on the same thread, which blocked forever
in g_cond_wait() on widgetready (only rigctl_run() signals it). The
socket was never closed, so the next Engage reused the dead socket.
Calling GTK widget functions off the main thread is also undefined
behavior in GTK3.
This does the cleanup on the worker thread (engaged flag, errcnt,
socket, timer) and moves the GTK updates to the main thread via
g_idle_add(), blocking the toggled handler so rig_engaged_cb()'s
user-initiated disengage branch does not run twice.
Tested with a fake rigctld-protocol server that is killed and
restarted. Before: deadlock, the "MAX_ERROR_COUNT reached" message is
never logged, and no reconnect happens. After: clean disengage, and
the next Engage connects to the restarted server and completes the
handshake. Builds on current master (GTK 3.24.41, GLib 2.80.0); the
g_signal_handlers_block_by_func calls give the usual -Wpedantic
function-pointer cast warning.