[Kimchi-devel] [PATCH V2] add a synchronous function with timeout to execute command

[Zhou Zheng Sheng]

on 2014/01/10 22:10, shaohef at linux.vnet.ibm.com wrote:
> From: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
> 
> We need a common function to execute shell command.
> We also need timeout when execute shell command.
> 
> A threading.Timer is used to send signal.SIGKILL to kill the command
> when timeout.
> 
> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
> Signed-off-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
> ---
>  src/kimchi/utils.py | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py
> index af245c6..c7165ce 100644
> --- a/src/kimchi/utils.py
> +++ b/src/kimchi/utils.py
> @@ -23,6 +23,7 @@
> 
>  import cherrypy
>  import os
> +import subprocess
>  import urllib2
> 
> 
> @@ -30,6 +31,7 @@ from cherrypy.lib.reprconf import Parser
> 
> 
>  from kimchi import config
> +from threading import Timer
> 
> 
>  kimchi_log = cherrypy.log.error_log
> @@ -96,3 +98,44 @@ def check_url_path(path):
>          return False
> 
>      return True
> +
> +
> +def run_command(cmd, timeout=None):

I think it will be useful if you can add a docstring here explaining
that timeout is a float number in seconds.

> +    def kill_proc(proc, timeout_flag):
> +        timeout_flag[0] = True
> +        proc.kill()
> +

Though it's rare, but I think it's still possible that the sub-process
exits before the timeout, but the timer expires just before we execute
the timer.cancel(). There is always a small time window between
"communicate()" and "cancel()". It's possible that Python switches
context to the timer thread during this time window, and it would except
"OSError: [Errno 3] No such process" here. I think it's better to except
OSError and swallow the exception. Which means the following.

def kill_proc(proc, timeout_flag):
    try:
        proc.kill()
    except OSError:
        pass
    else:
        timeout_flag[0] = True

> +    proc = None
> +    timer = None
> +    timeout_flag = [False]
> +
> +    try:
> +        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
> +                                stderr=subprocess.PIPE)
> +        if timeout is not None:
> +            timer = Timer(timeout, kill_proc, [proc, timeout_flag])

As far as I test, timer threads do not automatically set the .daemon
attribute to True. The .daemon of a thread is False by default [1], and
it prevents the process from exiting. I think we should set .daemon to
True for timer threads, and they get killed if the process is stopping.

[1] http://docs.python.org/2/library/threading.html#thread-objects
> +            timer.start()
> +
> +        out, error = proc.communicate()
> +        kimchi_log.debug("Run command: '%s'", " ".join(cmd))
> +
> +        if out or error:
> +            kimchi_log.debug("out:\n %s\nerror:\n %s", out, error)
> +
> +        if proc.returncode == -9 and timeout_flag[0] is True:

I think you can safely drop "is True", just "if ... and timeout_flag[0]"
is also correct and shorter.

If you applied my suggestions excepting and swallowing OSError in
kill_proc(), you might also be able to drop the "returncode == -9". In
all, I think it may be simplified as the following.

if timeout_flag[0]:
    kimchi_log.error("subprocess executed timeout for %s seconds, "
                   "return code %s" % (timeout, proc.returncode))

> +            kimchi_log.error("subprocess is killed by signal.SIGKILL "
> +                             "for timeout %s seconds", timeout)
> +
> +        return out, error, proc.returncode, timeout_flag[0]
> +    except Exception as e:
> +        msg = "Failed to run command: %s." % " ".join(cmd)
> +        msg = msg if proc is None else msg + "\n  error code: %s."
> +        kimchi_log.error("%s\n  %s", msg, e)
> +
> +        if proc:
> +            return out, error, proc.returncode, timeout_flag[0]
> +        else:
> +            return None, None, None, timeout_flag[0]
> +    finally:
> +        if timer and not timeout_flag[0]:
> +            timer.cancel()
> 


-- 
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397