fix: Prevent deadlock when stopping PLC with plugins#76
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two critical issues that occur when stopping the PLC runtime: a deadlock when S7Comm plugin clients are connected, and a hang when Python plugins are not initialized.
Changes:
- Reordered plugin stop sequence to occur before mutex acquisition in
unload_plc_program() - Added
Py_IsInitialized()guards around all Python GIL operations to prevent calling Python functions when Python isn't initialized
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/src/plc_app/plc_state_manager.c | Moved plugin_driver_stop() call before mutex acquisition to prevent deadlock with S7Comm plugin |
| core/src/drivers/plugin_driver.c | Added Python initialization checks before GIL operations in init, start, stop, restart, and destroy functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (python_initialized) | ||
| { | ||
| PyGILState_Release(local_gstate); | ||
| PyEval_RestoreThread(main_tstate); |
There was a problem hiding this comment.
The main_tstate variable is only set in plugin_driver_start() but is being used in plugin_driver_destroy(). If plugin_driver_destroy() is called without a prior call to plugin_driver_start(), or if plugin_driver_start() was not called with Python initialized, main_tstate will be uninitialized, leading to undefined behavior when calling PyEval_RestoreThread().
| PyEval_RestoreThread(main_tstate); | |
| if (main_tstate != NULL) | |
| { | |
| PyEval_RestoreThread(main_tstate); | |
| } |
|
|
||
| PyGILState_STATE local_gstate = PyGILState_Ensure(); | ||
| // Only acquire Python GIL if we have Python plugins and Python is initialized | ||
| PyGILState_STATE local_gstate = 0; |
There was a problem hiding this comment.
Initializing PyGILState_STATE to 0 is incorrect. PyGILState_STATE is an opaque type and should not be assumed to have a valid zero value. Declare it uninitialized and only assign it when PyGILState_Ensure() is called.
| PyGILState_STATE local_gstate = 0; | |
| PyGILState_STATE local_gstate; |
| 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; |
There was a problem hiding this comment.
Initializing PyGILState_STATE to 0 is incorrect. PyGILState_STATE is an opaque type and should not be assumed to have a valid zero value. Declare it uninitialized and only assign it when PyGILState_Ensure() is called.
| PyGILState_STATE local_gstate = 0; | |
| PyGILState_STATE local_gstate; |
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Summary
Problem
Issue 1: Deadlock with S7Comm plugin
When stopping the PLC while an S7Comm client was connected, the runtime would deadlock:
unload_plc_program()tried to acquirebuffer_mutexbuffer_mutexduring client read operationsIssue 2: Hang without Python plugins
When stopping the PLC with no Python plugins enabled (or with only native plugins like S7Comm), the runtime would hang because Python GIL functions were called unconditionally even when Python wasn't initialized.
Solution
Fix 1: Reorder plugin stop and mutex acquisition (plc_state_manager.c)
Fix 2: Guard Python GIL operations (plugin_driver.c)
Added
has_python_plugin && Py_IsInitialized()checks before all Python GIL operations in:plugin_driver_init()plugin_driver_start()plugin_driver_stop()plugin_driver_restart()plugin_driver_destroy()Test plan
🤖 Generated with Claude Code