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

Using mysqld_multi via sh kills mysqladmin grandchildren with SIGHUP before they have a chance to do anything #266

Closed
hoelzro opened this issue Jul 9, 2015 · 1 comment
Labels

Comments

@hoelzro
Copy link

hoelzro commented Jul 9, 2015

I found this issue when writing a MySQL helper script that uses mysqld_multi. The problematic line looks like this:

sh.mysqld_multi('stop', '1')

mysqld_multi is an interesting script that forks off a mysqladmin process for each start/stop action, but doesn't wait for them to finish before exiting itself. As a result, the controlling terminal that sh creates for mysqld_multi (which is the controlling terminal for its mysqladmin children) is closed when sh cleans up after invoking mysqld_multi, and the mysqladmin grandchildren processes are sent a SIGHUP by the kernel.

Here is a script that reproduces the behavior without involving MySQL scripts:

import sh
from tempfile import NamedTemporaryFile
from textwrap import dedent
import time

with \
    NamedTemporaryFile() as output_file, \
    NamedTemporaryFile() as child_file, \
    NamedTemporaryFile() as parent_file:

    child_file.write(dedent('''
    import signal
    import sys
    import time

    with open({output_file}, 'w') as f:
        def handle_sighup(signum, frame):
            f.write('got signal %d' % signum)
            sys.exit(signum)
        signal.signal(signal.SIGHUP, handle_sighup)
        time.sleep(5)
        f.write('made it!\\n')
    '''.format(output_file=repr(output_file.name))))
    child_file.flush()

    parent_file.write(dedent('''
    import os
    import time

    os.spawnlp(os.P_NOWAIT, 'python', 'python', {child_file})
    time.sleep(2) # give child a chance to set up
    '''.format(child_file=repr(child_file.name))))

    parent_file.flush()

    sh.python(parent_file.name)
    time.sleep(10)

    print output_file.readlines()[0] # should print 'made it'
amoffat pushed a commit that referenced this issue Oct 13, 2016
@amoffat
Copy link
Owner

amoffat commented Oct 13, 2016

After a lot of reading, I think the right move here is to only give an sh subprocess a controlling terminal IFF we are using _tty_in=True (which is False by default). My rationale is that if a process doesn't have a tty for stdin, then it doesn't make sense for it to have a controlling terminal (because, how could you send signals from the controlling terminal if you cannot input through a tty). This solves grandchild processes from receiving SIGHUP (because there is no ctty that hangs up). Additonally, if you do choose to launch a process with _tty_in=True, then you're launching the process as if it were a shell, so any subprocesses launched within that shell should receive SIGHUP, just like if they were launched in the foreground from a shell, and the shell hangs up.

e1d2a2c

this will go out with the release-1.2 release. interested if you have any additional thoughts

@amoffat amoffat closed this as completed Oct 13, 2016
@amoffat amoffat modified the milestone: 1.2 Oct 25, 2016
@amoffat amoffat removed the 1.2 label Oct 25, 2016
0-wiz-0 added a commit to NetBSD/pkgsrc-wip that referenced this issue Dec 12, 2016
*   added `_out` and `_out_bufsize` validator [#346](amoffat/sh#346)
*   bugfix for internal stdout thread running when it shouldn't [#346](amoffat/sh#346)

*   regression bugfix on timeout [#344](amoffat/sh#344)
*   regression bugfix on `_ok_code=None`

*   further improvements on cpu usage

*   regression in cpu usage [#339](amoffat/sh#339)

*   fd leak regression and fix for flawed fd leak detection test [#337](amoffat/sh#337)

*   support for `io.StringIO` in python2

*   added support for using raw file descriptors for `_in`, `_out`, and `_err`
*   removed `.close()`ing `_out` handler if FIFO detected

*   composed commands no longer propagate `_bg`
*   better support for using `sys.stdin` and `sys.stdout` for `_in` and `_out`
*   bugfix where `which()` would not stop searching at the first valid executable found in PATH
*   added `_long_prefix` for programs whose long arguments start with something other than `--` [#278](amoffat/sh#278)
*   added `_log_msg` for advanced configuration of log message [#311](amoffat/sh#311)
*   added `sh.contrib.sudo`
*   added `_arg_preprocess` for advanced command wrapping
*   alter callable `_in` arguments to signify completion with falsy chunk
*   bugfix where pipes passed into `_out` or `_err` were not flushed on process end [#252](amoffat/sh#252)
*   deprecated `with sh.args(**kwargs)` in favor of `sh2 = sh(**kwargs)`
*   made `sh.pushd` thread safe
*   added `.kill_group()` and `.signal_group()` methods for better process control [#237](amoffat/sh#237)
*   added `new_session` special keyword argument for controlling spawned process session [#266](amoffat/sh#266)
*   bugfix better handling for EINTR on system calls [#292](amoffat/sh#292)
*   bugfix where with-contexts were not threadsafe [#247](amoffat/sh#195)
*   `_uid` new special keyword param for specifying the user id of the process [#133](amoffat/sh#133)
*   bugfix where exceptions were swallowed by processes that weren't waited on [#309](amoffat/sh#309)
*   bugfix where processes that dupd their stdout/stderr to a long running child process would cause sh to hang [#310](amoffat/sh#310)
*   improved logging output [#323](amoffat/sh#323)
*   bugfix for python3+ where binary data was passed into a process's stdin [#325](amoffat/sh#325)
*   Introduced execution contexts which allow baking of common special keyword arguments into all commands [#269](amoffat/sh#269)
*   `Command` and `which` now can take an optional `paths` parameter which specifies the search paths [#226](amoffat/sh#226)
*   `_preexec_fn` option for executing a function after the child process forks but before it execs [#260](amoffat/sh#260)
*   `_fg` reintroduced, with limited functionality.  hurrah! [#92](amoffat/sh#92)
*   bugfix where a command would block if passed a fd for stdin that wasn't yet ready to read [#253](amoffat/sh#253)
*   `_long_sep` can now take `None` which splits the long form arguments into individual arguments [#258](amoffat/sh#258)
*   making `_piped` perform "direct" piping by default (linking fds together).  this fixes memory problems [#270](amoffat/sh#270)
*   bugfix where calling `next()` on an iterable process that has raised `StopIteration`, hangs [#273](amoffat/sh#273)
*   `sh.cd` called with no arguments no changes into the user's home directory, like native `cd` [#275](amoffat/sh#275)
*   `sh.glob` removed entirely.  the rationale is correctness over hand-holding. [#279](amoffat/sh#279)
*   added `_truncate_exc`, defaulting to `True`, which tells our exceptions to truncate output.
*   bugfix for exceptions whose messages contained unicode
*   `_done` callback no longer assumes you want your command put in the background.
*   `_done` callback is now called asynchronously in a separate thread.
*   `_done` callback is called regardless of exception, which is necessary in order to release held resources, for example a process pool
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants