Skip to content

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

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.

Sign up for GitHub

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

Closed
addaleax wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6acc5e
test: fix flaky test-vm-timeout
addaleax Jun 22, 2016
39aea05
vm: test for abort condition of current invocation
addaleax Jun 22, 2016
ce5f4dc
Revert "test: mark test-vm-timeout flaky on windows"
addaleax Jun 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
vm: test for abort condition of current invocation 
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.
  • Loading branch information
@addaleax
addaleax committed Jun 23, 2016
commit 39aea05db08877a2321ad2820f08a76d3c0f0d90
21 changes: 18 additions & 3 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,14 +836,17 @@ class ContextifyScript : public BaseObject {

Local<Value> result;
bool timed_out = false;
bool received_signal = false;
if (break_on_sigint && timeout != -1) {
Watchdog wd(env->isolate(), timeout);
SigintWatchdog swd(env->isolate());
result = script->Run();
timed_out = wd.HasTimedOut();
received_signal = swd.HasReceivedSignal();
} else if (break_on_sigint) {
SigintWatchdog swd(env->isolate());
result = script->Run();
received_signal = swd.HasReceivedSignal();
} else if (timeout != -1) {
Watchdog wd(env->isolate(), timeout);
result = script->Run();
Expand All @@ -852,14 +855,26 @@ class ContextifyScript : public BaseObject {
result = script->Run();
}

if (try_catch.HasCaught() && try_catch.HasTerminated()) {
env->isolate()->CancelTerminateExecution();
if (try_catch.HasCaught()) {
if (try_catch.HasTerminated())
env->isolate()->CancelTerminateExecution();

// 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) {
env->ThrowError("Script execution timed out.");
} else {
} else if (received_signal) {
env->ThrowError("Script execution interrupted.");
}

// If there was an exception thrown during script execution, re-throw it.
// If one of the above checks threw, re-throw the exception instead of
// letting try_catch catch it.
// If execution has been terminated, but not by one of the watchdogs from
// this invocation, this will re-throw a `null` value.
try_catch.ReThrow();

return false;
}

Expand Down
3 changes: 2 additions & 1 deletion src/node_watchdog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void SigintWatchdog::Dispose() {


SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
: isolate_(isolate), destroyed_(false) {
: isolate_(isolate), received_signal_(false), destroyed_(false) {
// Register this watchdog with the global SIGINT/Ctrl+C listener.
SigintWatchdogHelper::GetInstance()->Register(this);
// Start the helper thread, if that has not already happened.
Expand All @@ -120,6 +120,7 @@ void SigintWatchdog::Destroy() {


void SigintWatchdog::HandleSigint() {
received_signal_ = true;
isolate_->TerminateExecution();
}

Expand Down
2 changes: 2 additions & 0 deletions src/node_watchdog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,13 @@ class SigintWatchdog {
void Dispose();

v8::Isolate* isolate() { return isolate_; }
bool HasReceivedSignal() { return received_signal_; }

@bnoordhuis bnoordhuis Jun 22, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could be const. It's considered idiomatic to name simple getters after their properties (i.e. received_signal(), although has_received_signal() is arguably a better name) but the current name is not wrong.

EDIT: isolate() could have been const too but isn't either. Never mind then.

void HandleSigint();
private:
void Destroy();

v8::Isolate* isolate_;
bool received_signal_;
bool destroyed_;
};

Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-vm-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,26 @@ assert.throws(function() {
vm.runInNewContext('runInVM(10)', context, { timeout: 10000 });
throw new Error('Test 5 failed');
}, /Script execution timed out./);

// Test 6: Nested vm timeouts, outer timeout is shorter and fires first.
assert.throws(function() {
const context = {
runInVM: function(timeout) {
vm.runInNewContext('while(true) {}', context, { timeout: timeout });
}
};
vm.runInNewContext('runInVM(10000)', context, { timeout: 100 });
throw new Error('Test 6 failed');
}, /Script execution timed out./);

// Test 7: Nested vm timeouts, inner script throws an error.
assert.throws(function() {
const context = {
runInVM: function(timeout) {
vm.runInNewContext('throw new Error(\'foobar\')', context, {
timeout: timeout
});
}
};
vm.runInNewContext('runInVM(10000)', context, { timeout: 100000 });
}, /foobar/);