vm: add support for timeout argument #4115

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants

Added a watchdog class which executes a timer in a separate event loop in
a separate thread that will terminate v8 execution if it expires.

Added timeout argument to functions in vm module which use the watchdog
if a non-zero timeout is specified.

@langpavel langpavel and 2 others commented on an outdated diff Oct 11, 2012

test/simple/test-vm-run-timeout.js
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var vm = require('vm');
+
+assert.throws(function() {
+ try {
+ vm.runInThisContext('while(true) {}', '', 100);
+ } catch (e) {
+ if (e === null)
@langpavel

langpavel Oct 11, 2012

Well, this looks really weird. null is not Error instance, you should change this strange exception.

@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

Does it actually work? I thought the thing with V8::TerminateExecution() was that JS cannot catch it?

@langpavel

langpavel Oct 11, 2012

Is null used as a hack?

@apaprocki

apaprocki Oct 11, 2012

I can find out more from the v8 guys. I see it throw null through observation, so perhaps either the docs are incorrect or there is something else going on.

@apaprocki

apaprocki Oct 11, 2012

Ah, nevermind. The null exception is due to the TryCatch in node_script.cc not checking IsExecutionTerminating. Once that is done, the call simply returns without throwing like it is supposed to.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 11, 2012

src/node_script.cc
@@ -415,7 +424,14 @@ void WrappedScript::Initialize(Handle<Object> target) {
if (output_flag == returnResult) {
- result = script->Run();
+ if (timeout) {
+ Watchdog wd;
+ wd.beginTimer(timeout);
+ result = script->Run();
+ // Watchdog is destroyed upon success.
@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

Superfluous comment. Besides, it's also destroyed on error.

@apaprocki

apaprocki Oct 11, 2012

I can remove the comment. I removed wd.dispose() because it is also superfluous, but that means that if anyone ever adds code later on immediately following the script->Run() inside the same block, the watchdog will run longer than it is supposed to. The comment was meant more towards making sure the reader knows that either the block needs to end right there or wd.dispose() should be put back in prior to any following statements.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 11, 2012

src/node_watchdog.cc
+
+
+Watchdog::Watchdog()
+ : loop_(NULL)
+ , timer_started_(false)
+ , thread_created_(false)
+ , destroyed_(false) {
+}
+
+
+Watchdog::~Watchdog() {
+ destroy();
+}
+
+
+void Watchdog::dispose() {
@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

Style: method names start with a capital (see the Google C++ guide).

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 11, 2012

src/node_watchdog.cc
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+#include "node_watchdog.h"
+
+namespace node {
+
+using v8::V8;
+
+
+Watchdog::Watchdog()
+ : loop_(NULL)
+ , timer_started_(false)
+ , thread_created_(false)
+ , destroyed_(false) {
+}
@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

Is there a reason not to use RAII here?

@apaprocki

apaprocki Oct 11, 2012

Since I put the use of Watchdog inside the if, I can get rid of beginTimer and simply make the constructor initialize it.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 11, 2012

src/node_watchdog.cc
+ uv_timer_stop(&timer_);
+ }
+
+ if (loop_) {
+ uv_loop_delete(loop_);
+ }
+
+ if (thread_created_) {
+ uv_thread_join(&thread_);
+ }
+
+ destroyed_ = true;
+}
+
+
+int Watchdog::beginTimer(int64_t ms) {
@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

Use uint64_t if ms is always >= 0.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 11, 2012

src/node_watchdog.cc
+ return -1;
+ }
+ timer_started_ = true;
+
+ rc = uv_thread_create(&thread_, &Watchdog::run, this);
+ if (rc) {
+ return -1;
+ }
+ thread_created_ = true;
+
+ return 0;
+}
+
+
+void Watchdog::run(void* arg) {
+ Watchdog* wd = reinterpret_cast<Watchdog*>(arg);
@bnoordhuis

bnoordhuis Oct 11, 2012

Owner

static_cast

Owner

bnoordhuis commented Oct 11, 2012

One potential issue I see with this change is that it requires all TryCatch sites to check if V8::IsExecutionTerminating(). That's both tedious and error prone (because someone is going to forget it someday).

I see roughly 8 TryCatch sites in the code. Does it make sense to create a NodeTryCatch which checks IsExecutionTerminating() in its hasCaught()? You still run the risk of someone committing a TryCatch directly, but hopefully that would be easier to spot in a code review.

Separately, I noticed that node_script.cc assumes that if result.IsEmpty(), that try_catch has an exception inside it without checking try_catch.HasCaught(). Is that a bug?

robraux commented Oct 22, 2012

Apologies if this isn't the place to comment, but I'm very interested in this functionality. If there's anything I can do to help test or verify, please let me know and it's yours.

I have a change in to Chromium (V8) to add an API to resume execution after it has been terminated. That will allow this feature to work correctly, letting the vm module throw a real error if timeout occurs. This will have to wait until that change can be reviewed (and hopefully landed).

robraux commented Oct 22, 2012

@apaprocki can you link that request, or mailing thread request so those of us interested can monitor the V8 patch request?

V8 issue: http://code.google.com/p/v8/issues/detail?id=2361
Chromium review: http://codereview.chromium.org/11142013/

Hasn't been reviewed yet, so have to wait to see if there are any issues.

Owner

bnoordhuis commented Oct 22, 2012

I see roughly 8 TryCatch sites in the code. Does it make sense to create a NodeTryCatch which checks IsExecutionTerminating() in its hasCaught()?

That's probably a good idea.

Separately, I noticed that node_script.cc assumes that if result.IsEmpty(), that try_catch has an exception inside it without checking try_catch.HasCaught(). Is that a bug?

Well, it works the way it should in that it returns an empty handle when Script::Run() fails for some reason, e.g. because of an OOM condition - though that may be by accident, not by design.

Can one of the admins verify this patch?

The patch to V8 5 comments up still needs to be finished. The original attempt had to be backed out because it caused some failures on Linux IIRC. I will revisit it and submit a modified patch. Only once the support is added in V8 will this be possible in node.

robraux commented Mar 14, 2013

Thanks for the update. I remain very interested in this patch as well.

@apaprocki apaprocki vm: add support for timeout argument
Added a watchdog class which executes a timer in a separate event loop in
a separate thread that will terminate v8 execution if it expires.

Added timeout argument to functions in vm module which use the watchdog
if a non-zero timeout is specified.
99d8309

@bnoordhuis I pushed a new version of the code for this which is dependent on the upstream v8 change mentioned above. I just wanted to get some eyeballs on it while the v8 stuff is being reviewed.

If anyone is interested in playing with it, I created an integration branch with both this change and the v8 changes at apaprocki/node@1eb8663

(If the SHA link isn't helpful, that is apaprocki/node watchdog-integration branch https://github.com/apaprocki/node/tree/watchdog-integration)

Patch landed in v8, https://codereview.chromium.org/11142013/

Just have to wait for this to percolate up to the v8 in the node repo.

robraux commented Mar 21, 2013

This is fantastic news! Thanks for putting this work in @apaprocki , I can't wait to make solid use of it.

@bnoordhuis bnoordhuis commented on the diff Apr 22, 2013

src/node_script.cc
@@ -345,7 +357,18 @@ void WrappedScript::Initialize(Handle<Object> target) {
? args[filename_index]->ToString()
: String::New("evalmachine.<anonymous>");
- const int display_error_index = args.Length() - 1;
+ uint64_t timeout = 0;
+ const int timeout_index = filename_index + 1;
+ if (timeout_flag == useTimeout && args.Length() > timeout_index) {
+ if (!args[timeout_index]->IsUint32()) {
+ return ThrowException(Exception::TypeError(
+ String::New("needs an unsigned integer 'ms' argument.")));
+ }
+ timeout = args[timeout_index]->ToUint32()->Value();
@bnoordhuis

bnoordhuis Apr 22, 2013

Owner

s/ToUint32()->Value()/Uint32Value()/

Owner

bnoordhuis commented Apr 22, 2013

@apaprocki The PR looks fine to me. Do you know when the V8 guys will reland your patch?

@bnoordhuis It was supposed to reland in a week, but I guess that didn't happen -- pinging again now.

@bnoordhuis Landed in r14378 -- just have to wait for that to wind up in the v8 import here.

isaacs commented Apr 22, 2013

Should this be in the 0.12 or the 1.0 bucket?

Owner

bnoordhuis commented Apr 22, 2013

I'd be okay with v0.12 if the V8 patch lands in a stable V8 release before our v0.12 release but I don't think that's going to be the case.

isaacs commented Apr 22, 2013

I feel similarly ok. I'll tag it 0.12, if we get the V8 patch in time, then ok, if not, we can push it back.

Owner

bnoordhuis commented Apr 29, 2013

Landed in c081809. Thanks, Andrew.

(EDIT: In case anyone is wondering, the V8 patch landed in 3.18.3 and we're at 3.18.4 now.)

bnoordhuis closed this Apr 29, 2013

There are some cases in which the timeout causes node to crash. I created a new issue related to this patch: #5564

This was referenced Jul 29, 2013

This functionality is broken in the latest 0.10.18 (and in a couple of other versions I tried).
I tried:

var vm = require("vm");
console.log("started");
try {
    vm.runInNewContext("while(true) {}", {}, "loop", 1000);
} catch (e) {
    console.log("error: ", e);
}
console.log("finished");

I see started in the console and then the server hangs.

Running Windows 8.

Owner

bnoordhuis commented Sep 12, 2013

@anderslyman It's not broken in v0.10 because that patch never landed in v0.10 in the first place. If you click on the link to the commit, you'll see that the only tags/branches it applies to are v0.11.x ones.

Ah, I see, thank you kindly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment