test: add --abort-on-timeout option to test.py by misterdjules · Pull Request #11086 · nodejs/node · GitHub

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.

Refs: #11026

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

/cc @nodejs/testing

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

That is a very good question. Enabling core files on any platform will not generate additional core files for tests that use the --abort-on-uncaught-exception command line flag.

The tests that are using the --abort-on-uncaught-exception flag are written so that the node processes that end up running with that flag are run with child_process.exec and the command line used prepends ulimit -c 0 &&, disabling core file generation for these processes on most platforms.

I'm saying on most platforms because I'm not sure about platforms that I'm not familiar with, like AIX. Maybe @mhdawson can confirm?

On SmartOS, it's a bit different than Linux and OSX in the sense that ulimit -c 0 doesn't necessarily prevent any core file from being generated. Other system-wide settings can still allow core files to be generated when ulimit -c outputs 0. However, in that case, what happens on that platform is that a directory path where all core files are stored is setup, and a cron job cleans up that directory periodically.

Now, another use case with the changes in this PR is that, when tests times out on a developer's machine, core files could be generated since not all tests run with ulimit -c 0. The default on a lot of Linux distributions and on OSX, AFAIK, is to set the core file size limit to 0, which means no core file would be generated. However we can't really rely on this for these platforms, as developers can change their limit settings, or for the defaults not to change.

Thus, I would suggest to add support for a new --abort-on-timeout command line option for tools/test.py that, if set, would make test.py use SIGABRT instead of SIGTERM in KillProcessWithID. That new command line option could be used by the test-ci make target, and would make tests that timeout generate a core file.

This way, individual developers running tests on their development machine would never have any core file generated, even if they change their OS' core file size limit, but core files would be generated when appropriate for tests running on the CI platform.

How does that sound?

I like the suggestion of --abort-on-timeout command line option for tools/test.py. I would also give us flexibility to capture/not capture the core file depending on the platform as this could be add/not added in the specific subjob for each platform.

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

@mhdawson

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

Thanks for the confirmation!

Sending SIGABRT unconditionally like the PR currently does isn't very sociable. --abort-on-timeout seems like a good middle ground.

LGTM, but we should get a review from @nodejs/python as well.

Updated the changes according to recent discussions. I also updated the original comment and the title of this PR since the latest update changes its nature. Please take a look.

@misterdjules It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

LGTM with style nits.

Can you add an extra blank line here?

Done!

Indent by four spaces here.

Do we not have a linter for the python files? Not sure if eslint can handle it.

We don't. We don't maintain much python code, though. Pointing out the occasional style issue isn't much of a burden.

Done!

@gibfahn

It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

My intention was to set the TEST_CI_ARGS environment variable for the Jenkins job for which we want to enable this feature. For now, that would be for SmartOS only. Then if there's a need/desire to enable this across all platforms, we can indeed update the test-ci make target.

Does that make sense?

@misterdjules That does make sense, I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

So what's the reason not to enable this on other (non-win) platforms?

CI: https://ci.nodejs.org/job/node-test-pull-request/6170/

@gibfahn

I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

Change and change back as in "commit and push" these changes to the repository? I don't think I'd want to test this change by committing/reverting, when we can test it by setting TEST_CI_ARGS, which is how I'd like us to use this functionality at least for now (see below).

So what's the reason not to enable this on other (non-win) platforms?

The reason for not enabling this on other non-windows platforms is that it's not clear whether all CI machines/platforms are correctly setup for storing and cleaning up core files. AFAIK, the SmartOS machines are correctly setup with nodejs/build#492.

I wouldn't mind to enable this on all non-windows platforms, but it seems introducing it gradually doesn't have any cons and allows us to not disrupt the CI platform too much.

What do you think?

Okay, fair enough. It's just that it's not really possible to run CI on this feature before landing right?

@gibfahn

It's just that it's not really possible to run CI on this feature before landing right?

It is possible to test it. I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

SGTM

Unfortunately, this PR introduced a regression, which is documented and fixed by #11153. My apologies.

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: #11086 Ref: #11026 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

#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: #11086 PR-URL: #11153 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs#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: nodejs#11086 PR-URL: nodejs#11153 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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: nodejs#11086 Ref: nodejs#11026 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>