Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Stop container when signals captured #478

Closed
wants to merge 1 commit into from

Conversation

lamcw
Copy link

@lamcw lamcw commented Mar 15, 2024

The current atexit hook that stops (and optionally remove) containers in the Docker daemon would not work because of limitations of the atexit module: the handlers are not executed if the Python interpreter is

killed by a signal not handled by Python, when a Python fatal internal error is detected, or when os._exit() is called.
https://docs.python.org/3/library/atexit.html

To handle shutdowns of containers completely (for e.g. when the tests are terminated via SIGINT / SIGTERM), we'd need to register signal handlers for them as well.

This kind of acts as an interim solution while #314 is being implemented.

@@ -154,3 +160,28 @@ def _stop_container(container: Container) -> None:
pass
except Exception as ex:
LOGGER.warning("failed to shut down container %s with image %s: %s", container.id, container.image, ex)


def _register_signal_handler(handler: Callable[[], None], signals: Iterable) -> None:
Copy link
Author

Choose a reason for hiding this comment

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

A more advanced version of signal.signal that does not overwrite existing signal handler registered to signal.

@santi
Copy link
Collaborator

santi commented Mar 15, 2024

This issue will be resolved when #314 lands, which removes the atexit hook completely and removes the dependency on GC to cleanup danglings containers

@alexanderankin
Copy link
Member

going to close since reaper v1 has landed, and even reaper v2 (only difference being auto_remove=True (aka docker run --rm) so the instances dont hang out and haunt your system 👻)

if any other issues with stopping containers or anything else, just give a shout! @lamcw

@lamcw lamcw deleted the stop-containers-on-signal branch March 24, 2024 22:25
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