vm: fix nested timeouts with inverse order + fix flaky test-vm-timeout#7373
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added the c++ 
MemberAuthor
addaleax commented Jun 22, 2016
src/node_contextify.cc Outdated
Contributor
There was a problem hiding this comment.
IMHO it would look better to have these as separate declarations.
MemberAuthor
There was a problem hiding this comment.
Changed it to separate declarations
Member
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! 🎉 🎈 ✨ |
Contributor
mscdex commented Jun 22, 2016
If this is fixing the flaky test, there should be a commit that removes the flaky status too. |
addaleaxforce-pushed the vm-nested-timeouts-checking branch from 93dc504 to 37ca1ddCompareMemberAuthor
addaleax commented Jun 22, 2016
Updated + added revert of #7359 |
src/node_contextify.cc Outdated
Member
There was a problem hiding this comment.
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.
MemberAuthor
There was a problem hiding this comment.
@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.
Member
bnoordhuis commented Jun 22, 2016
LGTM with two suggestions. |
addaleaxforce-pushed the vm-nested-timeouts-checking branch from 37ca1dd to 71d0dc7CompareMemberAuthor
addaleax commented Jun 22, 2016
Member
Trott commented Jun 23, 2016
Looks like you did them right to me. |
src/node_contextify.cc Outdated
Contributor
There was a problem hiding this comment.
Could this just be an if - else if instead of nested ifs?
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.
This reverts commit f34caa9.
addaleaxforce-pushed the vm-nested-timeouts-checking branch from 71d0dc7 to ce5f4dcCompareMemberAuthor
addaleax commented Jun 25, 2016
One more CI: https://ci.nodejs.org/job/node-test-commit/3845/ |
MemberAuthor
addaleax commented Jun 25, 2016
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit that referenced this pull request Jun 25, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Merged
Contributor
MylesBorins commented Jul 11, 2016
@addaleax lts? |
MemberAuthor
addaleax commented Jul 11, 2016
@thealphanerd Yes, but only the |
Contributor
MylesBorins commented Jul 11, 2016
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Merged
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This reverts commit f34caa9. PR-URL: #7373 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
vm, test
Description of change
Fix#6727:
test-vm-timeoutwas 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 theErrorobject that would be created. Increasing the outer timeout won’t have any adverse effects anyway./cc @bnoordhuis ?