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

PTR: Promises are back #9

Closed
AlinaNova21 opened this issue May 3, 2017 · 6 comments
Closed

PTR: Promises are back #9

AlinaNova21 opened this issue May 3, 2017 · 6 comments

Comments

@AlinaNova21
Copy link
Contributor

Promises were disabled in #6 to prevent timeout skipping, with PTR on Node 7.9 Promises can be restored and used via the new async/await feature as seen below, this effectively readds the Promise class to global.

    let promise = async function(){ return '' }
    global.Promise = promise().constructor
@artch
Copy link
Contributor

artch commented May 3, 2017

Do we have any reliable method to disable async/await mechanic in Node.js?

@tedivm
Copy link

tedivm commented May 3, 2017

Since the node devs have marked this issue as "wontfix"[1] I'm getting the impression they're never going to resolve it. This may be one of those areas that needs an explicit rule telling people not to do it, with a punishment for those who do and maybe a mechanism on code upload to flag code for review by your team that may be breaking the rules. Otherwise I feel you're going to be playing whack-a-mole as people find ways to bypass things.

[1] nodejs/node#3020

@AlinaNova21
Copy link
Contributor Author

AFAIK theres no way to disable without recompiling node and/or v8 to disable it, even then I'm not sure it cna be disabled without modifying sources.

@artch
Copy link
Contributor

artch commented May 5, 2017

We're trying to look into threaded architecture now. If we manage to separate event loops for every user, it will not only allow to control what user is executing within his loop, but also to isolate GC for this single user.

@artch
Copy link
Contributor

artch commented May 9, 2017

@laverdet managed to patch Node.js for us that makes timeout and SIGINT work with pending Promises and other scheduled async code. Here is the patch:

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index ff66ffdaaa..c19d8e242e 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -857,22 +857,27 @@ class ContextifyScript : public BaseObject {
     Local<Value> result;
     bool timed_out = false;
     bool received_signal = false;
+    env->isolate()->RunMicrotasks();
     if (break_on_sigint && timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       SigintWatchdog swd(env->isolate());
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       timed_out = wd.HasTimedOut();
       received_signal = swd.HasReceivedSignal();
     } else if (break_on_sigint) {
       SigintWatchdog swd(env->isolate());
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       received_signal = swd.HasReceivedSignal();
     } else if (timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       result = script->Run();
+      env->isolate()->RunMicrotasks();
       timed_out = wd.HasTimedOut();
     } else {
       result = script->Run();
+      env->isolate()->RunMicrotasks();
     }
 
     if (try_catch->HasCaught()) {
@@ -896,6 +901,16 @@ class ContextifyScript : public BaseObject {
       try_catch->ReThrow();
 
       return false;
+    } else if (timed_out || received_signal) {
+      // `isolate->IsExecutionTerminating()` returns false which I suspect is a
+      // bug in v8, but `CancelTerminateExecution()` still works
+      env->isolate()->CancelTerminateExecution();
+      if (timed_out) {
+        env->ThrowError("Script execution timed out.");
+      } else {
+        env->ThrowError("Script execution interrupted.");
+      }
+      return false;
     }
 
     if (result.IsEmpty()) {

We're now testing this patch on Node.js 7.10.0 on the PTR.

@artch
Copy link
Contributor

artch commented May 11, 2017

This patch is deployed to production.

@artch artch closed this as completed May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants