add JS promise test, fix possible bugs in event loop handling

Author: LanderlYoung

.github/workflows/unit_tests.yml

@@ -265,4 +265,4 @@ jobs:
           cd build
           # exclude failed tests
           # --no-experimental-fetch config from https://github.com/emscripten-core/emscripten/issues/16915
-          node --no-experimental-fetch UnitTests.js '--gtest_filter=-ThreadPool.*:EngineScopeTest.ExitEngine:EngineScopeTest.TwoThreads:EngineScopeTest.ThreadLocal:MessageQueue.Interrupt:MessageQueue.Shutdown:MessageQueue.ShutdownNow:MessageQueue.FullAndPostInsideLoopQueue:ReferenceTest.WeakGc:ReferenceTest.WeakGc:ReferenceTest.GlobalNotClear:ReferenceTest.GlobalOnEngineDestroy:ReferenceTest.WeakOnEngineDestroy:ReferenceTest.WeakNotClrear:ManagedObjectTest.EngineDispose:ManagedObjectTest.FunctionCallback:PressureTest.All:EngineTest.JsPromiseTest:ShowCaseTest.SetTimeout'
+          node --no-experimental-fetch UnitTests.js '--gtest_filter=-ThreadPool.*:EngineScopeTest.ExitEngine:EngineScopeTest.TwoThreads:EngineScopeTest.ThreadLocal:MessageQueue.Interrupt:MessageQueue.Shutdown:MessageQueue.ShutdownNow:MessageQueue.FullAndPostInsideLoopQueue:ReferenceTest.WeakGc:ReferenceTest.WeakGc:ReferenceTest.GlobalNotClear:ReferenceTest.GlobalOnEngineDestroy:ReferenceTest.WeakOnEngineDestroy:ReferenceTest.WeakNotClrear:ManagedObjectTest.EngineDispose:ManagedObjectTest.FunctionCallback:PressureTest.All:EngineTest.JsPromiseTest:EngineTest.JsPromiseTest2:ShowCaseTest.SetTimeout'

VERSION

@@ -1 +1 @@
-3.6.0
+3.6.1

backend/QuickJs/QjsEngine.cc

@@ -191,9 +191,10 @@ void QjsEngine::destroy() noexcept {
   delete this;
 }
 
-void QjsEngine::scheduleTick() {
+void QjsEngine::triggerTick() {
   bool no = false;
-  if (tickScheduled_.compare_exchange_strong(no, true)) {
+  if (!isDestroying() && JS_IsJobPending(runtime_) &&
+      tickScheduled_.compare_exchange_strong(no, true)) {
     utils::Message tick(
         [](auto& m) {
           auto eng = static_cast<QjsEngine*>(m.ptr0);
@@ -203,9 +204,7 @@ void QjsEngine::scheduleTick() {
           }
           eng->tickScheduled_ = false;
         },
-        [](auto& m) {
-
-        });
+        nullptr);
     tick.ptr0 = this;
     tick.tag = this;
     queue_->postMessage(tick);
@@ -263,7 +262,7 @@ Local<Value> QjsEngine::eval(const Local<String>& script, const Local<Value>& so
   }
   qjs_backend::checkException(ret);
 
-  scheduleTick();
+  triggerTick();
 
   return Local<Value>(ret);
 }

backend/QuickJs/QjsEngine.h

@@ -140,7 +140,7 @@ class QjsEngine : public ScriptEngine {
   /**
    * similar to js_std_loop
    */
-  void scheduleTick();
+  void triggerTick();
 
   void extendLifeTimeToNextLoop(JSValue value);
 

backend/QuickJs/QjsLocalReference.cc

@@ -332,7 +332,7 @@ Local<Value> Local<Function>::callImpl(const Local<Value>& thiz, size_t size,
         qjs_backend::checkException(ret);
       });
 
-  engine.scheduleTick();
+  engine.triggerTick();
   return qjs_interop::makeLocal<Value>(ret);
 }
 

backend/QuickJs/QjsScope.cc

@@ -30,6 +30,7 @@ EngineScopeImpl::EngineScopeImpl(QjsEngine &current, QjsEngine *prev)
 }
 
 EngineScopeImpl::~EngineScopeImpl() {
+  current_->triggerTick();
   current_->runtimeLock_.unlock();
   if (previous_) {
     previous_->runtimeLock_.lock();

backend/QuickJs/QjsValue.cc

@@ -47,7 +47,7 @@ Local<Object> Object::newObjectImpl(const Local<Value>& type, size_t size,
     qjs_backend::checkException(ret);
   });
 
-  engine.scheduleTick();
+  engine.triggerTick();
   return qjs_interop::makeLocal<Object>(ret);
 }
 

backend/V8/V8Platform.cc

@@ -102,11 +102,25 @@ class MessageQueueTaskRunner : public v8::TaskRunner {
 
  private:
   void scheduleTask(std::unique_ptr<v8::Task> task, double delay_in_seconds = 0) {
-    script::utils::Message s([](auto& msg) { static_cast<v8::Task*>(msg.ptr0)->Run(); },
-                             [](auto& msg) {
-                               using deleter = std::unique_ptr<v8::Task>::deleter_type;
-                               deleter{}(static_cast<v8::Task*>(msg.ptr0));
-                             });
+    if (engine_->isDestroying()) {
+      return;
+    }
+
+    script::utils::Message s(
+        [](auto& msg) {
+          auto engine = static_cast<V8Engine*>(msg.tag);
+          EngineScope scope(engine);
+          try {
+            static_cast<v8::Task*>(msg.ptr0)->Run();
+          } catch (const Exception& e) {
+            // this should not happen, all JS exceptions should be handled by V8
+            abort();
+          }
+        },
+        [](auto& msg) {
+          using deleter = std::unique_ptr<v8::Task>::deleter_type;
+          deleter{}(static_cast<v8::Task*>(msg.ptr0));
+        });
     s.name = "SchedulePump";
     s.ptr0 = task.release();
     s.tag = engine_;

src/version.h

@@ -19,10 +19,10 @@
 
 // ScriptX version config files
 // auto generated from the file VERSION
-#define SCRIPTX_VERSION_STRING "3.6.0"
+#define SCRIPTX_VERSION_STRING "3.6.1"
 #define SCRIPTX_VERSION_MAJOR 3
 #define SCRIPTX_VERSION_MINOR 6
-#define SCRIPTX_VERSION_PATCH 0
+#define SCRIPTX_VERSION_PATCH 1
 
 namespace script {
 

test/src/EngineTest.cc

@@ -135,26 +135,56 @@ TEST_F(EngineTest, LuaBuiltIns) {
 
 #ifdef SCRIPTX_LANG_JAVASCRIPT
 TEST_F(EngineTest, JsPromiseTest) {
-  EngineScope scope(engine);
-
-  int value = 0;
-  auto setValue = Function::newFunction([&value](int val) { value = val; });
-  engine->set("setValue", setValue);
-  engine->eval(
-      u8R"(
-        const promise = new Promise((resolve, reject) => {
+  int value = -1;
+  {
+    EngineScope scope(engine);
+    auto setValue = Function::newFunction([&value](int val) {
+      // force new line
+      value = val;
+    });
+    engine->set("setValue", setValue);
+    engine->eval(
+        u8R"(
+        new Promise((resolve, reject) => {
+            resolve(0);
+        }).then(() => {
+          setValue(0);
+          new Promise((resolve, reject) => {
             resolve(1);
-        });
-
-        promise.then(num => {
+          }).then(num => {
             setValue(num);
+          });
         });
     )");
+  }
 
   engine->messageQueue()->shutdown(true);
   engine->messageQueue()->loopQueue(utils::MessageQueue::LoopType::kLoopAndWait);
   EXPECT_EQ(value, 1);
 }
+
+TEST_F(EngineTest, JsPromiseTest2) {
+  int value = -1;
+  {
+    EngineScope scope(engine);
+    auto setValue = Function::newFunction([&value](int val) { value = val; });
+    engine->set("setValue", setValue);
+    engine->eval(
+        u8R"(
+        new Promise((resolve, reject) => {
+            resolve(0);
+        }).then(() => {
+          setValue(0);
+          throw error(); // throws in promise callback
+          setValue(1);
+        });
+    )");
+  }
+
+  engine->messageQueue()->shutdown(true);
+  engine->messageQueue()->loopQueue(utils::MessageQueue::LoopType::kLoopAndWait);
+  EXPECT_EQ(value, 0);
+}
 #endif
 
 }  // namespace script::test