Skip to content

fix: Fixed directory cleanup after terminated by signal (#3905)#3913

Open
alvaro-osvaldo-tm wants to merge 7 commits intomkdocs:masterfrom
alvaro-osvaldo-tm:master
Open

fix: Fixed directory cleanup after terminated by signal (#3905)#3913
alvaro-osvaldo-tm wants to merge 7 commits intomkdocs:masterfrom
alvaro-osvaldo-tm:master

Conversation

@alvaro-osvaldo-tm
Copy link

  • Now the directory is cleanup when receive any 'terminate' class signal
  • The 'Server' class have an attribute 'is_active' to know the server state

@alvaro-osvaldo-tm
Copy link
Author

alvaro-osvaldo-tm commented Jan 27, 2025

Problem

Bug #3905 , when received a signal SIGTERM the directory cleanup is ignored due the execution not reach the 'clean up code'.

Solution

I 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.

Test

A shell script for a end-to-end test is available to validate the behaviour.
test-3905.txt

Considerations

  • Perhaps the function 'handle_signal' should put outside the 'serve.py' file to avoid 'SOLID' principles violation
  • The 'shutdown' function code perhaps should be put into the 'Server' class , method 'shutdown'.

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

…#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]>
Signed-off-by: Álvaro Osvaldo <[email protected]>
@alvaro-osvaldo-tm
Copy link
Author

It's ready, removed SIGQUIT and SIGUSR signals. Let me know if you need anything else.

  • Sorry for take so long , It wasn't clear to me if was me that should remove the SIGQUIT and SIGUSR signals.
  • Also, I accidentally merged no related code in the same pull request, so I had to made a 'force push'

@alvaro-osvaldo-tm alvaro-osvaldo-tm marked this pull request as ready for review February 6, 2025 17:51
Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously, could you add some tests @alvaro-osvaldo-tm?

@alvaro-osvaldo-tm
Copy link
Author

As mentioned previously, could you add some tests @alvaro-osvaldo-tm?

Sure, until February 25th, tuesday, I will send the pull request.

@alvaro-osvaldo-tm
Copy link
Author

alvaro-osvaldo-tm commented Feb 25, 2025

Hi, the test was made

  • I was careful for Windows cases , but still there is an issue I need to check in a windows machine, I don't have one at moment so I will do it this week.
  • The test was in a new class 'Shutdown_by_signal_tests' because required an infrastructure to locate the 'mkdocs serve' directory. Because it's not possible to define the temporary directory as parameter in 'mkdocs server' shell command, so I need to check every '/tmp' directory to found.
  • Exists many 'sleeps' and checks to prevent race conditions when running tests

I tried to make the test code readable , if need , I'm available for revision.

@pawamoy
Copy link
Contributor

pawamoy commented Feb 26, 2025

Thank you @alvaro-osvaldo-tm 🚀 Let me know when you've been able to handle the Windows issue you mentioned.

@alvaro-osvaldo-tm
Copy link
Author

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
configure the 'shutdown callback' and other to dispatch a 'shutdown' signal for testing

Something like in the 'serve.py'

    def shutdown() -> None:
        if not server.is_active:
            return

        log.info("Shutting down...")

        try:
            server.shutdown()
        finally:
            config.plugins.on_shutdown()

        if isdir(site_dir):
            shutil.rmtree(site_dir)

event_handler = Event_Handler_Factory.new()
event_handler.registry_shutdown( lambda: shutdown )

And something like in 'shutdown_by_signal_tests.py' testing code:

event_handler = Event_Dispatch_Factory.new()
event_handler.shutdown( mkdocs_pid )

for signal in event_handler.get_supported_signals():
   event_handler.dispatch( mkdocs_pid , signal )
   ...
   self.assertTrue( was_cleaned )

And the factories make the selection for the correct implementation

class Event_Handler_Factory:

  @staticmethod
  def new() -> IEvent_Handler:

    if sys.platform == 'win32':
     return Event_Handler_For_Win32()

     return Event_Handler_For_POSIX()


class Event_Dispatch_Factory:

  @staticmethod
  def new() -> IEvent_Dispatch:

    if sys.platform == 'win32':
     return Event_Dispatch_For_Win32()

     return Event_Dispatch_For_POSIX()

Did you like I implement it ?
No problem if the decisions change after the implementation.

I ask because is slightly different from the current mkdocs code patterns.

@pawamoy
Copy link
Contributor

pawamoy commented Mar 12, 2025

Seems like a good plan to me 🙂 I'd like to get @tomchristie's opinion too.
Thank you so much for your work on this, and for your patience @alvaro-osvaldo-tm 🙏

@orias
Copy link

orias commented Feb 27, 2026

Is this still a known issue? I'm running into giant /tmp/ folders on mkdocs 1.6.1.

#3905

Image

@alvaro-osvaldo-tm
Copy link
Author

alvaro-osvaldo-tm commented Mar 3, 2026

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:

  • Separation of Concerns: mkdocs/commands/serve.py remains nearly unchanged, merely invoking a generic listener interface.
  • Traceability: OS-specific bugs will be isolated within their respective strategy classes.
  • Cognitive Load: Maintainers only need to review the interface contract rather than complex conditional logic per OS.

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


Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants