screeps / driver

ISC License
26 stars 36 forks source link

PTR: Promises are back #9

Closed AlinaNova21 closed 7 years ago

AlinaNova21 commented 7 years ago

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 commented 7 years ago

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

tedivm commented 7 years ago

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] https://github.com/nodejs/node/issues/3020

AlinaNova21 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

This patch is deployed to production.