FACT_core icon indicating copy to clipboard operation
FACT_core copied to clipboard

Draft: Fix shutdown

Open maringuu opened this issue 2 years ago • 1 comments

With this PR only the "root" process receives the SIGINT signal. All other processes irgnore it. The complete_shutdown() that just kills the whole processgroup is removed. Also I extracted the parts of code that actually start a process in start methods. For me this seems better than starting right away in __init__.

Justifications for this change: The root process shuts down its children anyways. This means that the complete_shutdown did nothing but kill the one calling process, which was about to shutdown anyways. Also all children that inherited the signal handler didn't do anything but spam the logging since the run variable was not shared between processes anyway.

Closes #798

maringuu avatar Jun 20 '22 14:06 maringuu

I also removed FactBase. Quoting from the last commit:

FactBase is removed since it was not really used anyways. The calls to super() sometimes were missing because the super method did not fit. Removing inheritance simplifies the code.

maringuu avatar Jun 27 '22 07:06 maringuu

I don't like the idea to remove complete_shutdown. It is there for a reason: There are some cases where an exception in a process can lead to the backend (or frontend) not shutting down completely when the parent process exits. This leads to a "zombie" backend / frontend running in the background which can cause all kinds of weird problems (which I have encountered on multiple occasions in the past). Killing the process group was the only reliable way to shut down everything in case of an error.

If you just want to get rid of the "spammy" messages you could simply not log them from subprocesses:

from multiprocessing import current_process
...
    def shutdown_listener(self, signum, _):
        if is_main_process():
            logging.info(f'Received signal {signum}. Shutting down {self.PROGRAM_NAME}...')
            self.run = False

def is_main_process() -> bool:
    return current_process().name == 'MainProcess'

jstucke avatar Oct 05 '22 14:10 jstucke

Superseeded by #934 and #914

maringuu avatar Dec 22 '22 08:12 maringuu