2
\$\begingroup\$

I'm using a Python script to run a particular program that occasionally may need to be terminated (by receiving a SIGINT or SIGTERM) in which case a clean up process needs to be invoked (before killing the original process).

To test it I start the program in question and then I send a SIGINT to terminate it.

A signal handler is defined for SIGINT and SIGTERM that will launch a clean up program that takes as input a filepath output by the original process in its STDOUT. Afterwards it'll send a SIGTERM to the original process.

That clean up may hang in which case I raise a SIGALRM where I kill both the original process and the clean up process.

The problem is I'm not sure if I've covered all corner cases and I'm not really good at handling OS facilities (especially dealing with signals). For example, If I send SIGINT several times it terminates abruptly even though technically I have a flag to prevent SIGINT from being handled more than once.

EDIT: Included changes to the original code, such as the submission of subprocesses under process groups

Here's a stripped down version of the code:

import os
import sys
import signal
import subprocess

SUBMISSION_PROCESS = None # reference to the original process
CLEANUP_PROCESS    = None # reference to the clean up process
FILE_PATH          = '' # clean up process needs this from original process
HANDLER_CALLED     = False # to prevent signal handler from being called again

def kill_everything(signum, stack): # in case both processes hang...
    print 'Timed out, will kill all processes...'
    if SUBMIT_PROCESS:
        os.killpg(SUBMIT_PROCESS.pid, signal.SIGKILL)
    if CLEANUP_PROCESS:
        os.killpg(CLEANUP_PROCESS.pid, signal.SIGKILL)


def kill_submission_process(signum, stack):
    global HANDLER_CALLED
    global CLEANUP_PROCESS
    print 'Caught signal %d, please wait...' % signum

    if HANDLER_CALLED:
        print 'Already handling %d.' % signum
        return

    HANDLER_CALLED = True

    if FILE_PATH:
        stop_session_call = ['clean_up', '-arg_1', FILE_PATH]
        try:
            signal.alarm(120) # set a timeout for clean up session in case that fails too
            CLEANUP_PROCESS = subprocess.Popen(stop_session_call, preexec_fn=os.setsid)
            print 'Clean up process has started...'
            CLEANUP_PROCESS.communicate()
        except OSError as e:
            print >> sys.stderr, 'Unable to clean up session: an OSError was raised: %s' % e
    else:
        print >> sys.stderr, 'Unable to clean up session: file path not defined.'
    if SUBMISSION_PROCESS and SUBMISSION_PROCESS.poll() is None: # clean up original process if set and still running
        print 'Will clean up original submission process...'
        SUBMISSION_PROCESS.terminate()
        print 'Done, will quit.'
    raise SystemExit

if __name__ == '__main__':
    signal.signal(signal.SIGINT,  kill_submission_process)
    signal.signal(signal.SIGTERM, kill_submission_process)
    signal.signal(signal.SIGALRM, kill_everything)

    to_run = ['command', '-arg_1', 'param_1', '-arg_2', 'param_2']

    try:
        SUBMIT_PROCESS = subprocess.Popen(to_run, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, preexec_fn=os.setsid)

        # Trying to parse process output to identify a particular file path
        while True: 
            line = SUBMISSION_PROCESS.stdout.readline()
            if SUBMISSION_PROCESS.poll() != None and line == '': # if no more to read
                break
            line = line.rstrip('\n')
            print line
            if line[-4:] == '.ext': # line contains file path in question
                FILE_PATH = line # store it
                print 'File path found and saved.'
    except (OSError, IOError) as e:
        print >> sys.stderr, 'Exception raised while executing %s: %s.' % (' '.join(map(str, to_run)), e)
        raise SystemExit            
\$\endgroup\$
  • 1
    \$\begingroup\$ FWIW, there are specific rules about updating code to prevent answers being invalidated. In this case I don't think it'd be problematic to update the code generally, but you should not incorporate my (or anyone else's) suggestions as they invalidate answers. I've undone those aspects of the edit. \$\endgroup\$ – Veedrac Mar 25 '15 at 9:18
  • \$\begingroup\$ @Veedrac sorry, wasn't aware of this, will add a note in the post. \$\endgroup\$ – Nobilis Mar 25 '15 at 9:32
  • 1
    \$\begingroup\$ it seems like if you want to be able to cancel tests at any point, you should register handlers in those processes to do clean-up. \$\endgroup\$ – Veedrac Mar 26 '15 at 12:33
  • \$\begingroup\$ @Veedrac I unfortunately can't as all processes are instances of proprietary tools. My wrapper is the only customisable bit in any way. \$\endgroup\$ – Nobilis Mar 26 '15 at 12:44
3
\$\begingroup\$

UPPER_CASE is reserved for constants, and your globals assuredly aren't constants. If you wish for a cleaner solution, you could possibly make a class and pass in instance methods to signal.signal.

Your final raise SystemExit seems unneeded.

You can use line.endswith('.ext') instead of slicing.

It's hard to give more detailed analysis without a better understanding of what the code is trying to do.

\$\endgroup\$
  • \$\begingroup\$ Hi and thanks very much for your comments. The purpose of the code is to launch a process that runs a bunch of tests but a user may wish to cancel it, in which case a separate process is launched that cleans up data from those tests and then kills the original process. I've put them in upper case to make it obvious that they are globals but you are of course right that that's a convention for constants. \$\endgroup\$ – Nobilis Mar 25 '15 at 8:45
  • \$\begingroup\$ Actually, I subsequently updated the code to launch all subprocesses under a process group so that when I kill those, they kill anything that they spawn on their own, I'll update the code to reflect that. \$\endgroup\$ – Nobilis Mar 25 '15 at 8:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.