fix: Fixed directory cleanup after terminated by signal (#3905)#3913
fix: Fixed directory cleanup after terminated by signal (#3905)#3913alvaro-osvaldo-tm wants to merge 7 commits intomkdocs:masterfrom
Conversation
alvaro-osvaldo-tm
commented
Jan 27, 2025
- Now the directory is cleanup when receive any 'terminate' class signal
- The 'Server' class have an attribute 'is_active' to know the server state
ProblemBug #3905 , when received a signal SIGTERM the directory cleanup is ignored due the execution not reach the 'clean up code'. SolutionI wasn't able to move the 'clean up code' to make it available to both SIGTERM and SIGINT cases without side effects, so I created the function 'handle_signal' that handle both SIGTERM, SIGINT and other edge case for both Windows and Linux and realize the shutdown propertly. To avoid race conditions a property was created to 'Server' class to know if was yet stopped. TestA shell script for a end-to-end test is available to validate the behaviour. Considerations
|
099e8bf to
03a0eec
Compare
03a0eec to
1214692
Compare
…#3905) - Now the directory is cleanup when receive any 'terminate' class signal - The 'Server' class have an attribute 'is_active' to know the server state Signed-off-by: Álvaro Osvaldo <[email protected]>
1214692 to
8bef904
Compare
Signed-off-by: Álvaro Osvaldo <[email protected]>
|
It's ready, removed SIGQUIT and SIGUSR signals. Let me know if you need anything else.
|
pawamoy
left a comment
There was a problem hiding this comment.
So IMO it looks good. I'd just like to see some tests: they would open a subprocess of mkdocs serve with subprocess.Popen, and then send a signal to the subprocess with os.kill to assert that the process correctly terminates and cleaned up (or not) temporary directories.
@tomchristie with such tests would you be comfortable merging?
Signed-off-by: Álvaro Osvaldo <[email protected]>
pawamoy
left a comment
There was a problem hiding this comment.
As mentioned previously, could you add some tests @alvaro-osvaldo-tm?
Sure, until February 25th, tuesday, I will send the pull request. |
Signed-off-by: Álvaro Osvaldo <[email protected]>
Signed-off-by: Álvaro Osvaldo <[email protected]>
Signed-off-by: Álvaro Osvaldo <[email protected]>
|
Hi, the test was made
I tried to make the test code readable , if need , I'm available for revision. |
|
Thank you @alvaro-osvaldo-tm 🚀 Let me know when you've been able to handle the Windows issue you mentioned. |
|
Hi, it take me a while. For my surprise windows don't handle POSIX events smooth as Linux. In windows it handle CTRL_C_EVENT using specific 'Windows API' instead to handle SIGTERM using 'Python's events' library. I made this conclusion testing into an windows environment, reading some 'Python test cases' and more research. I have a proposal to enable the cleanup feature for both POSIX and Windows , while keeping the implementation simple and maintainable. It's use the factory pattern, the 'code' and the 'test' will request an implementation in the factories for the signal handling and receive one according the 'operation system'. Behind the factory there will be two implementations, one for 'POSIX' and other for 'Windows' with the methods to Something like in the 'serve.py' And something like in 'shutdown_by_signal_tests.py' testing code: And the factories make the selection for the correct implementation Did you like I implement it ? I ask because is slightly different from the current mkdocs code patterns. |
|
Seems like a good plan to me 🙂 I'd like to get @tomchristie's opinion too. |
|
Hi @pawamoy, @tomchristie. I propose refactoring the current implementation to decouple operating system (OS) logic from the core command. By applying the Strategy Pattern, we can isolate OS-specific interrupt handling into dedicated classes. Currently, the Linux implementation is ready. This architecture allows us to merge the feature now with a 'No-Op' strategy for Windows and macOS, preventing regressions while keeping the codebase open for future contributions. The modularity of this design makes implementing additional event handlers straightforward. Once the Linux version is merged, I can provide the Windows implementation as a follow-up PR, and possibly MacOS support later. Key benefits:
Below is a diagram for context: flowchart
subgraph InterruptStrategy["Interrupt Strategy"]
Observer
Handler
end
subgraph Serve["Serve.py"]
defServe["def Serve()"]
defShutdown["def shutdown()"]
end
subgraph Infrastructure
Linux
Windows
MacOS
end
defServe -- invoke --> Observer
Observer -- notify --> Handler
Infrastructure -.->|realizes| Observer
Handler -- callback --> defShutdown
|
