vm: fix nested timeouts with inverse order + fix flaky test-vm-timeout #7373

addaleax · 2016-06-22T21:56:07Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

vm, test

Description of change

Fix #6727:

I think @Trott is right in Investigate flaky test-vm-timeout.js #6727 to assume the changed error message stems from the inner timeout in the test not checking whether it has witnessed its own timeout or the outer one.
My best guess for why test-vm-timeout was flaky even before vm,repl: (add ability to) break on sigint/ctrl+c #6635 is that the outer timeout could interfere with handling of the inner timeout, e.g. during construction of the Error object that would be created. Increasing the outer timeout won’t have any adverse effects anyway.

/cc @bnoordhuis ?

addaleax · 2016-06-22T21:57:04Z

CI: https://ci.nodejs.org/job/node-test-commit/3819/

mscdex · 2016-06-22T22:24:15Z

src/node_contextify.cc

@@ -835,15 +835,17 @@ class ContextifyScript : public BaseObject {
    Local<Script> script = unbound_script->BindToCurrentContext();

    Local<Value> result;
-    bool timed_out = false;
+    bool timed_out = false, received_signal = false;


IMHO it would look better to have these as separate declarations.

Changed it to separate declarations

Trott · 2016-06-22T22:24:48Z

Green! The CI is green again! I'm reluctant to review C++ parts of Node.js (even though this looks pretty straightforward) and I'll leave that to people infinitely more familiar with Node.js's C++ pieces. But 💯 on having a fix for this! 🎉 🎈 ✨

mscdex · 2016-06-22T22:25:25Z

If this is fixing the flaky test, there should be a commit that removes the flaky status too.

addaleax · 2016-06-22T22:28:36Z

Updated + added revert of #7359

bnoordhuis · 2016-06-22T22:48:17Z

src/node_contextify.cc

      }
+
      try_catch.ReThrow();


Not a comment but an observation: it's not necessary to rethrow when calling ThrowError() (although it doesn't hurt either - ThrowException() silently updates the TryCatch exception) but it might be confusing to the casual reader what exactly gets rethrown.

@bnoordhuis Not sure I’m completely understanding you – the ReThrow() is necessary because otherwise the TryCatch would catch the Script execution timed out/interrupted errors.

But you’re right, it might be a good idea to explicitly state what exactly is thrown here, I’m adding a comment.

bnoordhuis · 2016-06-22T22:52:48Z

LGTM with two suggestions.

addaleax · 2016-06-22T23:19:53Z

Updated, stress tests (if I’ve managed to get everything right?): Linux, Windows

Trott · 2016-06-23T02:57:52Z

stress tests (if I’ve managed to get everything right?)

Looks like you did them right to me.

cjihrig · 2016-06-23T14:31:26Z

src/node_contextify.cc

+      // It is possible that execution was terminated by another timeout in
+      // which this timeout is nested, so check whether one of the watchdogs
+      // from this invocation is responsible for termination.
+      if (timed_out || received_signal) {


Could this just be an if - else if instead of nested ifs?

@cjihrig done, thanks

cjihrig · 2016-06-23T14:31:47Z

LGTM with one small comment.

Likely fix the flaky parallel/test-vm-timeout. Increase the outer timeout in the test checking for nested timeouts with `vm` scripts so that its firing won’t interfere with the inner timeout. Fixes: nodejs#6727

When a vm script aborted after a timeout/signal interruption, test whether the local timeout/signal watchdog was responsible for terminating the execution. Without this, when a shorter timer from an outer `vm.run*` invocation fires before an inner timeout, the inner timeout would throw an error instead of the outer one, but because it did not witness the timeout itself, it would assume the termination was the result of a signal interruption.

This reverts commit f34caa9.

addaleax · 2016-06-25T03:16:10Z

One more CI: https://ci.nodejs.org/job/node-test-commit/3845/

addaleax · 2016-06-25T03:58:34Z

Landed in efc3150, 61196de, 1d75987. Thanks for the reviews!

Likely fix the flaky parallel/test-vm-timeout. Increase the outer timeout in the test checking for nested timeouts with `vm` scripts so that its firing won’t interfere with the inner timeout. Fixes: #6727 PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a vm script aborted after a timeout/signal interruption, test whether the local timeout/signal watchdog was responsible for terminating the execution. Without this, when a shorter timer from an outer `vm.run*` invocation fires before an inner timeout, the inner timeout would throw an error instead of the outer one, but because it did not witness the timeout itself, it would assume the termination was the result of a signal interruption. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Likely fix the flaky parallel/test-vm-timeout. Increase the outer timeout in the test checking for nested timeouts with `vm` scripts so that its firing won’t interfere with the inner timeout. Fixes: #6727 PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a vm script aborted after a timeout/signal interruption, test whether the local timeout/signal watchdog was responsible for terminating the execution. Without this, when a shorter timer from an outer `vm.run*` invocation fires before an inner timeout, the inner timeout would throw an error instead of the outer one, but because it did not witness the timeout itself, it would assume the termination was the result of a signal interruption. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Likely fix the flaky parallel/test-vm-timeout. Increase the outer timeout in the test checking for nested timeouts with `vm` scripts so that its firing won’t interfere with the inner timeout. Fixes: #6727 PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a vm script aborted after a timeout/signal interruption, test whether the local timeout/signal watchdog was responsible for terminating the execution. Without this, when a shorter timer from an outer `vm.run*` invocation fires before an inner timeout, the inner timeout would throw an error instead of the outer one, but because it did not witness the timeout itself, it would assume the termination was the result of a signal interruption. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-07-11T23:48:06Z

@addaleax lts?

addaleax · 2016-07-11T23:51:11Z

@thealphanerd Yes, but only the test commit and the revert (if the original commit has been/will be backported, obviously).

MylesBorins · 2016-07-11T23:55:30Z

sounds a bit convoluted... would you be able to send a backport PR?

Likely fix the flaky parallel/test-vm-timeout. Increase the outer timeout in the test checking for nested timeouts with `vm` scripts so that its firing won’t interfere with the inner timeout. Fixes: nodejs#6727 PR-URL: nodejs#7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This reverts commit f34caa9. PR-URL: nodejs#7373 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>