fix: Python Function Block compilation and cleanup#91
Conversation
The iec_python.h header is included during compilation of generated PLC code. Python Function Blocks generate inline C code that calls getpid(), but the header only included sys/types.h (for pid_t) and was missing unistd.h (for getpid() declaration), causing compilation to fail with "implicit declaration of function 'getpid'" error. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The runtime crashed when stopping a PLC with Python Function Blocks that were actively printing to stdout. This happened because: 1. python_loader.c was compiled into libplc_*.so (not main executable) 2. Runner threads reading Python stdout were detached and never tracked 3. When dlclose() unloaded the library, runner threads crashed because their code (runner_thread function) was unmapped from memory Changes: - Track all Python blocks in an array with thread IDs, PIDs, and shm info - Use fork()/exec() instead of popen() to get the Python subprocess PID - Add python_blocks_cleanup() function that: - Sends SIGTERM to all Python processes (then SIGKILL if needed) - Joins all runner threads (they exit on EOF when Python dies) - Unmaps and unlinks shared memory regions - Removes Python script files - Call python_blocks_cleanup() before dlclose() in unload_plc_program() - Add null checks to LOG_INFO/LOG_ERROR macros for safety during cleanup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes compilation and runtime issues with Python Function Blocks by adding the missing <unistd.h> include and implementing proper cleanup to prevent crashes during PLC shutdown.
Changes:
- Added
<unistd.h>to resolvegetpid()implicit declaration - Implemented tracking of Python blocks with thread IDs, PIDs, and shared memory info
- Added
python_blocks_cleanup()function to properly terminate Python processes and join threads before library unload
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| core/src/plc_app/python_loader.c | Implements Python block tracking, replaces popen() with fork()/exec(), adds comprehensive cleanup function |
| core/src/plc_app/plc_state_manager.c | Calls python_blocks_cleanup() before dlclose() to prevent crashes |
| core/src/plc_app/include/iec_python.h | Adds <unistd.h> include and declares python_blocks_cleanup() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if cleanup started while we were blocked | ||
| if (python_cleanup_in_progress) | ||
| { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
The python_cleanup_in_progress flag is checked in both the while condition and immediately inside the loop. This is redundant - the inner check on lines 123-126 is unnecessary since the while condition will already exit the loop if python_cleanup_in_progress becomes true.
| // Check if cleanup started while we were blocked | |
| if (python_cleanup_in_progress) | |
| { | |
| break; | |
| } |
| return -1; | ||
| goto error_deactivate; | ||
| } | ||
| close(shm_in_fd); |
There was a problem hiding this comment.
The file descriptor shm_in_fd is closed here after successful mapping, but in the error paths (lines 224, 232) it's also closed before jumping to cleanup. This could lead to confusion. Consider setting shm_in_fd = -1 after closing it here so error paths can check if it's already closed.
| close(shm_in_fd); | |
| close(shm_in_fd); | |
| shm_in_fd = -1; |
| // Execute Python with unbuffered output | ||
| execlp("python3", "python3", "-u", script_name, (char *)NULL); | ||
|
|
||
| // If exec fails |
There was a problem hiding this comment.
The exit code 127 is typically reserved for 'command not found' in shells. While this makes sense here (exec failed), it would be clearer to log an error message before exiting. Since all file descriptors including stderr have been redirected to the pipe, the parent will never see why exec failed.
| // If exec fails | |
| // If exec fails, report the error through stderr (redirected to the pipe) | |
| { | |
| int saved_errno = errno; | |
| dprintf(STDERR_FILENO, | |
| "[Python loader] execlp(\"python3\") failed: %s\n", | |
| strerror(saved_errno)); | |
| } |
| pthread_mutex_unlock(&python_blocks_mutex); | ||
|
|
||
| // Give Python processes time to exit gracefully | ||
| usleep(100000); // 100ms |
There was a problem hiding this comment.
The hardcoded 100ms grace period for Python processes to exit is arbitrary and may be insufficient for processes with cleanup handlers or large state. Consider making this configurable or implementing a polling loop with a timeout to avoid unnecessary delay when processes exit quickly.
| } | ||
|
|
||
| // Wait for runner thread to exit (it will get EOF from closed pipe) | ||
| pthread_join(block->thread, NULL); |
There was a problem hiding this comment.
This call to pthread_join happens while holding python_blocks_mutex. If the runner thread is blocked in fgets() and hasn't noticed python_cleanup_in_progress yet, this could deadlock if the runner thread needs to acquire the same mutex. The mutex should be released before joining threads.
| pthread_join(block->thread, NULL); | |
| pthread_t thread = block->thread; | |
| pthread_mutex_unlock(&python_blocks_mutex); | |
| pthread_join(thread, NULL); | |
| pthread_mutex_lock(&python_blocks_mutex); |
| { | ||
| LOG_ERROR("[Python loader] Failed to write Python script: %s", strerror(errno)); | ||
| return -1; | ||
| goto error_deactivate; |
There was a problem hiding this comment.
When fopen() fails on line 191, the error jumps to error_deactivate which decrements python_block_count on line 329. However, the count was already incremented on line 186, so this is correct. But the error message doesn't inform the user that the slot has been freed, which could be helpful for debugging resource exhaustion issues.
| *(void **)(&python_cleanup) = | ||
| plugin_manager_get_symbol(pm, "python_blocks_cleanup"); |
There was a problem hiding this comment.
The double cast *(void **)(&python_cleanup) is used to suppress compiler warnings about converting between function and object pointers, but this is a POSIX extension not guaranteed by C standard. A more portable pattern would be to use a union to perform the type-punning, though this may be acceptable for this codebase if targeting POSIX systems exclusively.
| *(void **)(&python_cleanup) = | |
| plugin_manager_get_symbol(pm, "python_blocks_cleanup"); | |
| union | |
| { | |
| void *obj; | |
| void (*func)(void); | |
| } sym; | |
| sym.obj = plugin_manager_get_symbol(pm, "python_blocks_cleanup"); | |
| python_cleanup = sym.func; |
Summary
getpid()implicit declaration)Problem
Issue 1: Compilation failure
When uploading a PLC project with Python Function Blocks, compilation failed with:
Issue 2: Runtime crash on stop
When stopping the PLC while a Python Function Block was actively printing to stdout, the runtime crashed and was automatically restarted by the webserver, making it impossible to stop the PLC.
Root Cause
Issue 1:
iec_python.hincluded<sys/types.h>(forpid_t) but was missing<unistd.h>(forgetpid()).Issue 2:
python_loader.cis compiled intolibplc_*.so. Runner threads reading Python stdout were detached and never tracked. Whendlclose()unloaded the library, the threads crashed because their code (runner_threadfunction) was unmapped from memory.Changes
#include <unistd.h>toiec_python.hfork()/exec()instead ofpopen()to get Python subprocess PIDspython_blocks_cleanup()function that:python_blocks_cleanup()beforedlclose()inunload_plc_program()MAX_PYTHON_BLOCKSfrom 32 to 128Test plan
[Python]messages)[Python loader] Cleaning up...and[Python loader] Cleanup complete🤖 Generated with Claude Code