Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

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 addaleax added the vm Issues and PRs related to the vm subsystem. label Jun 22, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 22, 2016
@addaleax
Copy link
Member Author

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to separate declarations

@Trott
Copy link
Member

Trott commented Jun 22, 2016

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
Copy link
Contributor

mscdex commented Jun 22, 2016

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

@addaleax addaleax force-pushed the vm-nested-timeouts-checking branch from 93dc504 to 37ca1dd Compare June 22, 2016 22:27
@addaleax
Copy link
Member Author

Updated + added revert of #7359

}

try_catch.ReThrow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member

LGTM with two suggestions.

@addaleax addaleax force-pushed the vm-nested-timeouts-checking branch from 37ca1dd to 71d0dc7 Compare June 22, 2016 23:09
@addaleax
Copy link
Member Author

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

@Trott
Copy link
Member

Trott commented Jun 23, 2016

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

Looks like you did them right to me.

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig done, thanks

@cjihrig
Copy link
Contributor

cjihrig commented Jun 23, 2016

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.
@addaleax addaleax force-pushed the vm-nested-timeouts-checking branch from 71d0dc7 to ce5f4dc Compare June 23, 2016 14:55
@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

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

@addaleax addaleax closed this Jun 25, 2016
@addaleax addaleax deleted the vm-nested-timeouts-checking branch June 25, 2016 03:58
addaleax added a commit that referenced this pull request Jun 25, 2016
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>
addaleax added a commit that referenced this pull request Jun 25, 2016
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>
addaleax added a commit that referenced this pull request Jun 25, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@MylesBorins
Copy link
Contributor

@addaleax lts?

@addaleax
Copy link
Member Author

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

@MylesBorins
Copy link
Contributor

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

addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
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>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
This reverts commit f34caa9.

PR-URL: nodejs#7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This reverts commit f34caa9.

PR-URL: #7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants