From 783fd7d8262b012329cb162c7ae4cbfe92674e0d Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 14 Jan 2026 13:35:08 -0500 Subject: [PATCH 1/3] fix: Prevent deadlock when stopping PLC with plugins Two issues fixed: 1. Deadlock with S7Comm plugin (plc_state_manager.c): - unload_plc_program() tried to acquire buffer_mutex BEFORE stopping plugins, but S7Comm's RWArea callback holds this mutex during client reads, causing deadlock - Fix: Call plugin_driver_stop() before acquiring the mutex 2. Hang when stopping without Python plugins (plugin_driver.c): - plugin_driver_init/start/stop/destroy called Python GIL functions even when no Python plugins exist or Python wasn't initialized - Fix: Add has_python_plugin && Py_IsInitialized() checks before all Python GIL operations Co-Authored-By: Claude Opus 4.5 --- core/src/drivers/plugin_driver.c | 76 +++++++++++++++++++++++----- core/src/plc_app/plc_state_manager.c | 7 ++- 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 5bc20ae3..16fcde7b 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -281,7 +281,13 @@ int plugin_driver_init(plugin_driver_t *driver) return -1; } - PyGILState_STATE local_gstate = PyGILState_Ensure(); + // Only acquire Python GIL if we have Python plugins and Python is initialized + PyGILState_STATE local_gstate = 0; + int have_gil = has_python_plugin && Py_IsInitialized(); + if (have_gil) + { + local_gstate = PyGILState_Ensure(); + } // #chamdo a função init de cada plugin aqui for (int i = 0; i < driver->plugin_count; i++) @@ -306,7 +312,10 @@ int plugin_driver_init(plugin_driver_t *driver) fprintf(stderr, "Failed to generate runtime args for plugin: %s\n", plugin->config.name); - PyGILState_Release(local_gstate); + if (have_gil) + { + PyGILState_Release(local_gstate); + } return -1; } // Call the Python init function with proper capsule @@ -322,7 +331,10 @@ int plugin_driver_init(plugin_driver_t *driver) fprintf(stderr, "Python init function failed for plugin: %s\n", plugin->config.name); - PyGILState_Release(local_gstate); + if (have_gil) + { + PyGILState_Release(local_gstate); + } return -1; } Py_DECREF(result); @@ -338,6 +350,10 @@ int plugin_driver_init(plugin_driver_t *driver) { fprintf(stderr, "Failed to generate runtime args for native plugin: %s\n", plugin->config.name); + if (have_gil) + { + PyGILState_Release(local_gstate); + } return -1; } @@ -348,6 +364,10 @@ int plugin_driver_init(plugin_driver_t *driver) fprintf(stderr, "Native init function failed for plugin: %s (returned %d)\n", plugin->config.name, result); free_structured_args(args); + if (have_gil) + { + PyGILState_Release(local_gstate); + } return -1; } @@ -356,7 +376,10 @@ int plugin_driver_init(plugin_driver_t *driver) } } - PyGILState_Release(local_gstate); + if (have_gil) + { + PyGILState_Release(local_gstate); + } return 0; } @@ -375,8 +398,12 @@ int plugin_driver_start(plugin_driver_t *driver) return 0; } - gstate = PyGILState_Ensure(); - main_tstate = PyEval_SaveThread(); + // Only manage Python GIL if we have Python plugins and Python is initialized + if (has_python_plugin && Py_IsInitialized()) + { + gstate = PyGILState_Ensure(); + main_tstate = PyEval_SaveThread(); + } for (int i = 0; i < driver->plugin_count; i++) { @@ -465,7 +492,13 @@ int plugin_driver_stop(plugin_driver_t *driver) return 0; } - PyGILState_STATE local_gstate = PyGILState_Ensure(); + // Only acquire Python GIL if we have Python plugins and Python is initialized + PyGILState_STATE local_gstate; + int need_gil = has_python_plugin && Py_IsInitialized(); + if (need_gil) + { + local_gstate = PyGILState_Ensure(); + } // Signal all plugins to stop for (int i = 0; i < driver->plugin_count; i++) @@ -508,7 +541,10 @@ int plugin_driver_stop(plugin_driver_t *driver) // Plugin manager only handles destruction, not stopping } - PyGILState_Release(local_gstate); + if (need_gil) + { + PyGILState_Release(local_gstate); + } return 0; } @@ -531,7 +567,7 @@ int plugin_driver_restart(plugin_driver_t *driver) // Clean up plugins without destroying the driver // Note: No need for GIL here as stop() already handled Python operations - if (has_python_plugin) + if (has_python_plugin && Py_IsInitialized()) { gstate = PyGILState_Ensure(); for (int i = 0; i < driver->plugin_count; i++) @@ -581,17 +617,26 @@ void plugin_driver_destroy(plugin_driver_t *driver) if (driver->plugin_count == 0) { printf("[PLUGIN]: No plugins to destroy.\n"); + pthread_mutex_destroy(&driver->buffer_mutex); + free(driver); return; } - PyGILState_STATE local_gstate = PyGILState_Ensure(); + // Check if Python is initialized before any Python operations + int python_initialized = has_python_plugin && Py_IsInitialized(); + PyGILState_STATE local_gstate = 0; + + if (python_initialized) + { + local_gstate = PyGILState_Ensure(); + } plugin_driver_stop(driver); for (int i = 0; i < driver->plugin_count; i++) { plugin_instance_t *plugin = &driver->plugins[i]; - if (plugin->python_plugin) + if (plugin->python_plugin && python_initialized) { python_plugin_cleanup(plugin); } @@ -616,9 +661,12 @@ void plugin_driver_destroy(plugin_driver_t *driver) } } - PyGILState_Release(local_gstate); - PyEval_RestoreThread(main_tstate); - Py_FinalizeEx(); + if (python_initialized) + { + PyGILState_Release(local_gstate); + PyEval_RestoreThread(main_tstate); + Py_FinalizeEx(); + } pthread_mutex_destroy(&driver->buffer_mutex); diff --git a/core/src/plc_app/plc_state_manager.c b/core/src/plc_app/plc_state_manager.c index 678eb586..bfa9d95c 100644 --- a/core/src/plc_app/plc_state_manager.c +++ b/core/src/plc_app/plc_state_manager.c @@ -194,10 +194,15 @@ int unload_plc_program(PluginManager *pm) journal_cleanup(); log_info("Journal buffer cleaned up"); + // Stop plugins FIRST (before acquiring mutex) to prevent deadlock + // The S7Comm plugin's RWArea callback acquires buffer_mutex during + // client read operations. If we try to acquire the mutex before + // stopping the plugin, we can deadlock if a client is connected. + plugin_driver_stop(plugin_driver); + // Clear temporary pointers from image tables before unloading // This ensures clean state for the next program load plugin_mutex_take(&plugin_driver->buffer_mutex); - plugin_driver_stop(plugin_driver); image_tables_clear_null_pointers(); plugin_mutex_give(&plugin_driver->buffer_mutex); From bcac6dd05f7ccf74688983de3551170d93d576b5 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 14 Jan 2026 16:34:54 -0500 Subject: [PATCH 2/3] fix: Address Copilot review feedback - Remove unnecessary initialization of PyGILState_STATE to 0 (opaque type) - Add NULL check for main_tstate before PyEval_RestoreThread() to prevent undefined behavior if destroy is called without start Co-Authored-By: Claude Opus 4.5 --- core/src/drivers/plugin_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 16fcde7b..0e2337b8 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -282,7 +282,7 @@ int plugin_driver_init(plugin_driver_t *driver) } // Only acquire Python GIL if we have Python plugins and Python is initialized - PyGILState_STATE local_gstate = 0; + PyGILState_STATE local_gstate; int have_gil = has_python_plugin && Py_IsInitialized(); if (have_gil) { @@ -624,7 +624,7 @@ void plugin_driver_destroy(plugin_driver_t *driver) // Check if Python is initialized before any Python operations int python_initialized = has_python_plugin && Py_IsInitialized(); - PyGILState_STATE local_gstate = 0; + PyGILState_STATE local_gstate; if (python_initialized) { @@ -664,7 +664,10 @@ void plugin_driver_destroy(plugin_driver_t *driver) if (python_initialized) { PyGILState_Release(local_gstate); - PyEval_RestoreThread(main_tstate); + if (main_tstate != NULL) + { + PyEval_RestoreThread(main_tstate); + } Py_FinalizeEx(); } From 3b99b48d60846903bf3cdf4eaf39095e6fe6f0e0 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 14 Jan 2026 16:39:43 -0500 Subject: [PATCH 3/3] fix: Initialize PyGILState_STATE to PyGILState_LOCKED Use the explicit enum value PyGILState_LOCKED instead of 0 to satisfy compiler warning while being semantically correct. The variable is only used when have_gil/python_initialized is true, but the compiler's static analysis can't prove this across all control flow paths. Co-Authored-By: Claude Opus 4.5 --- core/src/drivers/plugin_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 0e2337b8..f7cd7fd8 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -282,7 +282,8 @@ int plugin_driver_init(plugin_driver_t *driver) } // Only acquire Python GIL if we have Python plugins and Python is initialized - PyGILState_STATE local_gstate; + // Initialize to PyGILState_LOCKED (0) to satisfy compiler warning + PyGILState_STATE local_gstate = PyGILState_LOCKED; int have_gil = has_python_plugin && Py_IsInitialized(); if (have_gil) { @@ -624,7 +625,8 @@ void plugin_driver_destroy(plugin_driver_t *driver) // Check if Python is initialized before any Python operations int python_initialized = has_python_plugin && Py_IsInitialized(); - PyGILState_STATE local_gstate; + // Initialize to PyGILState_LOCKED (0) to satisfy compiler warning + PyGILState_STATE local_gstate = PyGILState_LOCKED; if (python_initialized) {