From 82dab736f84dea8305d013128ba48dc6e0d1ad0d Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Tue, 31 Jan 2017 09:34:30 -0800 Subject: [PATCH 1/2] test: add --abort-on-timeout option to test.py Currently, when a process times out, it is terminated by sending it the SIGTERM signal. Sending SIGBART instead allows the operating system to generate a core file that can be investigated later using post-mortem debuggers such as llnode or mdb_v8. This can be very useful when investigating flaky tests that time out, since in that case the failure is difficult to reproduce, and being able to look at a core file makes a big difference. With these changes, passing the --abort-on-timeout command line option to tools/test.py now sends SIGABRT to processes timing out on all platforms but Windows. PR-URL: https://github.com/nodejs/node/pull/11086 Ref: https://github.com/nodejs/node/issues/11026 Reviewed-By: James M Snell Reviewed-By: Gibson Fahnestock Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Santiago Gimeno Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Michael Dawson Reviewed-By: Anna Henningsen --- tools/test.py | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/tools/test.py b/tools/test.py index 838f70a17d4fec..b511004a737490 100755 --- a/tools/test.py +++ b/tools/test.py @@ -564,11 +564,11 @@ def HasFailed(self): return execution_failed -def KillProcessWithID(pid): +def KillProcessWithID(pid, signal_to_send=signal.SIGTERM): if utils.IsWindows(): os.popen('taskkill /T /F /PID %d' % pid) else: - os.kill(pid, signal.SIGTERM) + os.kill(pid, signal_to_send) MAX_SLEEP_TIME = 0.1 @@ -587,6 +587,17 @@ def Win32SetErrorMode(mode): pass return prev_error_mode + +def KillTimedOutProcess(context, pid): + signal_to_send = signal.SIGTERM + if context.abort_on_timeout: + # Using SIGABRT here allows the OS to generate a core dump that can be + # looked at post-mortem, which helps for investigating failures that are + # difficult to reproduce. + signal_to_send = signal.SIGABRT + KillProcessWithID(pid, signal_to_send) + + def RunProcess(context, timeout, args, **rest): if context.verbose: print "#", " ".join(args) popen_args = args @@ -626,7 +637,7 @@ def RunProcess(context, timeout, args, **rest): while True: if time.time() >= end_time: # Kill the process and wait for it to exit. - KillProcessWithID(process.pid) + KillTimedOutProcess(context, process.pid) exit_code = process.wait() timed_out = True break @@ -647,7 +658,7 @@ def RunProcess(context, timeout, args, **rest): while exit_code is None: if (not end_time is None) and (time.time() >= end_time): # Kill the process and wait for it to exit. - KillProcessWithID(process.pid) + KillTimedOutProcess(context, process.pid) exit_code = process.wait() timed_out = True else: @@ -855,7 +866,7 @@ class Context(object): def __init__(self, workspace, buildspace, verbose, vm, expect_fail, timeout, processor, suppress_dialogs, - store_unexpected_output, repeat): + store_unexpected_output, repeat, abort_on_timeout): self.workspace = workspace self.buildspace = buildspace self.verbose = verbose @@ -866,6 +877,7 @@ def __init__(self, workspace, buildspace, verbose, vm, expect_fail, self.suppress_dialogs = suppress_dialogs self.store_unexpected_output = store_unexpected_output self.repeat = repeat + self.abort_on_timeout = abort_on_timeout def GetVm(self, arch, mode): if arch == 'none': @@ -1400,6 +1412,9 @@ def BuildOptions(): result.add_option('--repeat', help='Number of times to repeat given tests', default=1, type="int") + result.add_option('--abort-on-timeout', + help='Send SIGABRT instead of SIGTERM to kill processes that time out', + default=False, dest="abort_on_timeout") return result @@ -1579,7 +1594,8 @@ def Main(): processor, options.suppress_dialogs, options.store_unexpected_output, - options.repeat) + options.repeat, + options.abort_on_timeout) # First build the required targets if not options.no_build: reqs = [ ] From 3abf144fba54b03ce8a1b9fe30bcc265884905bf Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 3 Feb 2017 19:57:25 +0000 Subject: [PATCH 2/2] test: fix test.py command line options processing https://github.com/nodejs/node/pull/11086 had introduced a regression that broke command line options processing for tools/test.py. Basically, it made tools/test.py discard the command line argument that would be passed after `--abort-on-timeout`. For instance, when running: ``` $ python tools/test.py --abort-on-timeout path/to/some-test ``` all tests would be run because the last command line argument (`/path/to/some-test`) would be discarded. This change fixes this regression. Refs: https://github.com/nodejs/node/pull/11086 PR-URL: https://github.com/nodejs/node/pull/11153 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen --- tools/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/test.py b/tools/test.py index b511004a737490..42f25ae52201e5 100755 --- a/tools/test.py +++ b/tools/test.py @@ -1414,7 +1414,7 @@ def BuildOptions(): default=1, type="int") result.add_option('--abort-on-timeout', help='Send SIGABRT instead of SIGTERM to kill processes that time out', - default=False, dest="abort_on_timeout") + default=False, action="store_true", dest="abort_on_timeout") return result