From 57369ad3686f7158203257dda29b4ba2d598580d Mon Sep 17 00:00:00 2001 From: N6RFM Date: Wed, 23 Sep 2026 04:55:37 +0000 Subject: [PATCH] Fix self-deadlock and stale-socket reuse in radio control auto-disengage 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 #425 --- src/gtk-rig-ctrl.c | 53 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/src/gtk-rig-ctrl.c b/src/gtk-rig-ctrl.c index 80d5c59a..c171259a 100644 --- a/src/gtk-rig-ctrl.c +++ b/src/gtk-rig-ctrl.c @@ -2995,6 +2995,30 @@ static void rigctrl_open(GtkRigCtrl *data) } } +/* Marshals the auto-disengage UI update onto the main thread. The + actual cleanup (socket close, timer removal) is done by the caller + on the worker thread before this runs; this function only touches + GTK widgets, which must never be called from a non-main thread. The + "toggled" signal is blocked while updating the button so this does + not re-trigger rig_engaged_cb's own disengage branch (which expects + to run on user-initiated Engage/Disengage, on the main thread, and + would otherwise self-deadlock if reached from here -- see the + MAX_ERROR_COUNT handling in rigctl_run for why this exists). */ +static gboolean rig_auto_disengage_ui_cb(gpointer data) +{ + GtkRigCtrl *ctrl = GTK_RIG_CTRL(data); + + g_signal_handlers_block_by_func(ctrl->LockBut, rig_engaged_cb, ctrl); + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(ctrl->LockBut), FALSE); + g_signal_handlers_unblock_by_func(ctrl->LockBut, rig_engaged_cb, ctrl); + + gtk_widget_set_sensitive(ctrl->DevSel, TRUE); + gtk_widget_set_sensitive(ctrl->DevSel2, TRUE); + ctrl->rigctl_thread = NULL; + + return FALSE; +} + /* Communication thread for hamlib rigctld */ gpointer rigctl_run(gpointer data) { @@ -3085,17 +3109,34 @@ gpointer rigctl_run(gpointer data) /* perform error count checking */ if (t_ctrl->errcnt >= MAX_ERROR_COUNT) { - /* disengage device */ - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(t_ctrl->LockBut), - FALSE); - t_ctrl->engaged = FALSE; - t_ctrl->errcnt = 0; sat_log_log( SAT_LOG_LEVEL_ERROR, _("%s:%s: MAX_ERROR_COUNT (%d) reached. Disengaging device!"), __FILE__, __func__, MAX_ERROR_COUNT); - // g_print ("ERROR. WROPS = %d\n", ctrl->wrops); + /* Do the disengage cleanup directly here, on this thread, + instead of calling gtk_toggle_button_set_active(). That + call would synchronously re-enter this same thread via + rig_engaged_cb's disengage branch, which does + g_cond_wait() waiting for a signal that only the *else* + branch further up in this same function can send -- + code that can never run until this call returns. The + result was a permanent self-deadlock: this thread parks + forever, rigctrl_close() (which resets ctrl->sock to 0) + never runs, and every subsequent Engage silently reuses + the dead socket instead of reconnecting. */ + t_ctrl->engaged = FALSE; + t_ctrl->errcnt = 0; + + if (t_ctrl->sock > 0) + rigctrl_close(t_ctrl); + + if (t_ctrl->timerid) + remove_timer(t_ctrl); + + g_idle_add(rig_auto_disengage_ui_cb, t_ctrl); + + break; } // g_print (" WROPS = %d\n", ctrl->wrops);