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

manhole socket not removed when terminating a process #8

Open
nirs opened this issue Aug 31, 2014 · 6 comments
Open

manhole socket not removed when terminating a process #8

nirs opened this issue Aug 31, 2014 · 6 comments

Comments

@nirs
Copy link
Collaborator

nirs commented Aug 31, 2014

Although manhole register the removal function using atexit module, the removal function is not run when sending the process SIGTERM.

To reproduce:

  1. Create a script::

    import manhole
    import time
    manhole.install(socket_path='exit.manhole')
    time.sleep(30)

  2. Run the script in the backround

    python exit.py &

  3. And terminate it

    kill

The removal function is not called, and manhole socket is not removed.

Tested on Fedora 19.

This seems faulty implementation of atexit module.

Workarounds:

  • terminate the process using SIGINT
  • manually remove the socket in the application - easy when using socket_path option
@nirs
Copy link
Collaborator Author

nirs commented Sep 2, 2014

Adding manhole.uninstall() would be useful for application that want to ensure that the manhole is cleanup properly.

manhole.uninstall() should

  1. remove the socket from the file system. Connected client should not be affected by this.
  2. set a flag, so the manhole thread will exit
  3. wakeup the manhole thread, in case it is blocking on accept - I think that closing the manhole socket should work.

This of course will not help manhole inherited in a sub process, but it will be good enough for me, since I'm using socket_path.

nirs added a commit to nirs/python-manhole that referenced this issue Sep 6, 2014
manhole.install() registers an exit function removing the manhole
socket, but atexit function do not run when process is terminated by an
unhandled signal.

In the fork tests, we avoid this issue by terminating child processes
with SIGINT, however the test child processes, created by the
process_tests library, are terminate using SIGTERM in
TestProcess.__exit__.

Adding SIGTERM signal handler fixed this issue.

We probably need to document this as a workaround for issue ionelmc#8
@nirs
Copy link
Collaborator Author

nirs commented Sep 6, 2014

Workaround: define a signal hanlder for SIGTERM:

import signal
import sys

def handle_sigterm(signo, frame):
    print "Terminated"
    sys.exit(128 + signo)

signal.signal(signal.SIGTERM, handle_sigterm)

@ionelmc
Copy link
Owner

ionelmc commented Sep 6, 2014

Should this be part of the documentation ?

Thanks,
-- Ionel M.

On Sat, Sep 6, 2014 at 10:35 PM, Nir Soffer [email protected]
wrote:

Workaround: define a signal hanlder for SIGTERM:

import signal
import sys

def handle_sigterm(signo, frame):
print "Terminated"
sys.exit(128 + signo)

signal.signal(signal.SIGTERM, handle_sigterm)


Reply to this email directly or view it on GitHub
#8 (comment)
.

@nirs
Copy link
Collaborator Author

nirs commented Sep 6, 2014

I guess it should, maybe under "Known issues".

ionelmc added a commit that referenced this issue Sep 13, 2014
@nirs
Copy link
Collaborator Author

nirs commented Oct 19, 2014

I think we can register the signal handler in manhole, checking that no signal handler was installed before that:

if signal.getsignal(signal.SIGTERM) == signal.SIG_DFL:
   signal.signal(signal.SIGTERM, _handle_sigterm)

If a user install sigterm handler after that, we don't care, since atexit will be called. If the user did not install one, our handler will make sure the socket will be cleaned.

The only issue is that installing signals is possible only from the main thread, so manhole.install() will have to be called from the main thread. I don't think this is an issue since anyone will call this from the main thread anyway.

@nirs
Copy link
Collaborator Author

nirs commented Dec 23, 2017

I think this issue is resolved by #8, so we can close it now.

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

No branches or pull requests

2 participants