pr feedback

README.md

@@ -51,7 +51,7 @@ cmake --build cmake-build-debug --target integration-test-server
 Then tests can be run with `ctest` directly via:
 
 ```bash
-CTEST_OUTPUT_ON_FAILURE=1 ctest --test-dir cmake-build-debug -j8
+ctest --test-dir cmake-build-debug -j8 --output-on-failure
 ```
 
 Alternatively, the integration test server can be directly run with `wasmtime serve` via:

include/extension-api.h

@@ -40,17 +40,17 @@ class Engine {
 
   /**
    * Define a new builtin module
-   * 
+   *
    * The enumerable properties of the builtin object are used to construct
    * a synthetic module namespace for the module.
-   * 
+   *
    * The enumeration and getters are called only on the first import of
    * the builtin, so that lazy getters can be used to lazily initialize
    * builtins.
-   * 
+   *
    * Once loaded, the instance is cached and reused as a singleton.
    */
-  bool define_builtin_module(const char* id, HandleValue builtin);
+  bool define_builtin_module(const char *id, HandleValue builtin);
 
   /**
    * Treat the top-level script as a module or classic JS script.
@@ -64,21 +64,33 @@ class Engine {
   bool eval_toplevel(const char *path, MutableHandleValue result);
 
   /**
-   * Run the JS task queue and wait on pending tasks until there
-   * are no outstanding lifetimes to wait on.
+   * Run the async event loop as long as there's interest registered in keeping it running.
+   *
+   * Each turn of the event loop consists of three steps:
+   * 1. Run reactions to all promises that have been resolves/rejected.
+   * 2. Check if there's any interest registered in continuing to wait for async tasks, and
+   *    terminate the loop if not.
+   * 3. Wait for the next async tasks and execute their reactions
+   *
+   * Interest or loss of interest in keeping the event loop running can be signaled using the
+   * `Engine::incr_event_loop_interest` and `Engine::decr_event_loop_interest` methods.
+   *
+   * Every call to incr_event_loop_interest must be followed by an eventual call to
+   * decr_event_loop_interest, for the event loop to complete. Otherwise, if no async tasks remain
+   * pending while there's still interest in the event loop, an error will be reported.
    */
   bool run_event_loop();
 
   /**
-   * Add an event loop lifetime to track
+   * Add an event loop interest to track
    */
-  void incr_event_loop_lifetime();
+  void incr_event_loop_interest();
 
   /**
-   * Remove an event loop lifetime to track
+   * Remove an event loop interest to track
    * The last decrementer marks the event loop as complete to finish
    */
-  void decr_event_loop_lifetime();
+  void decr_event_loop_interest();
 
   /**
    * Get the JS value associated with the top-level script execution -

runtime/engine.cpp

@@ -418,12 +418,12 @@ bool api::Engine::run_event_loop() {
   return core::EventLoop::run_event_loop(this, 0);
 }
 
-void api::Engine::incr_event_loop_lifetime() {
-  return core::EventLoop::incr_event_loop_lifetime();
+void api::Engine::incr_event_loop_interest() {
+  return core::EventLoop::incr_event_loop_interest();
 }
 
-void api::Engine::decr_event_loop_lifetime() {
-  return core::EventLoop::decr_event_loop_lifetime();
+void api::Engine::decr_event_loop_interest() {
+  return core::EventLoop::decr_event_loop_interest();
 }
 
 bool api::Engine::dump_value(JS::Value val, FILE *fp) { return ::dump_value(CONTEXT, val, fp); }

runtime/event_loop.cpp

@@ -9,7 +9,8 @@
 
 struct TaskQueue {
   std::vector<api::AsyncTask *> tasks = {};
-  int lifetime_cnt = 0;
+  int interest_cnt = 0;
+  bool event_loop_running = false;
 
   void trace(JSTracer *trc) const {
     for (const auto task : tasks) {
@@ -39,53 +40,74 @@ bool EventLoop::cancel_async_task(api::Engine *engine, const int32_t id) {
 
 bool EventLoop::has_pending_async_tasks() { return !queue.get().tasks.empty(); }
 
-void EventLoop::incr_event_loop_lifetime() {
-  queue.get().lifetime_cnt++;
-}
+void EventLoop::incr_event_loop_interest() { queue.get().interest_cnt++; }
 
-void EventLoop::decr_event_loop_lifetime() {
-  MOZ_ASSERT(queue.get().lifetime_cnt > 0);
-  queue.get().lifetime_cnt--;
+void EventLoop::decr_event_loop_interest() {
+  MOZ_ASSERT(queue.get().interest_cnt > 0);
+  queue.get().interest_cnt--;
 }
 
-bool lifetime_complete() {
-  return queue.get().lifetime_cnt == 0;
-}
+inline bool interest_complete() { return queue.get().interest_cnt == 0; }
+
+inline void exit_event_loop() { queue.get().event_loop_running = false; }
 
 bool EventLoop::run_event_loop(api::Engine *engine, double total_compute) {
+  if (queue.get().event_loop_running) {
+    fprintf(stderr, "cannot run event loop as it is already running");
+    return false;
+  }
+  queue.get().event_loop_running = true;
   JSContext *cx = engine->cx();
 
   while (true) {
     while (js::HasJobsPending(cx)) {
       js::RunJobs(cx);
-      if (JS_IsExceptionPending(cx))
+      if (JS_IsExceptionPending(cx)) {
+        exit_event_loop();
         return false;
-      if (lifetime_complete())
+      }
+      if (interest_complete()) {
+        exit_event_loop();
         return true;
+      }
     }
 
-    auto tasks = &queue.get().tasks;
+    const auto tasks = &queue.get().tasks;
 
-    // Unresolved lifetime error -
-    // if there are no async tasks, and the lifetime was not complete
-    // then we cannot complete the lifetime
     if (tasks->size() == 0) {
+      fprintf(stderr, "event loop error - both task and job queues are empty, but expected "
+                      "operations did not resolve");
+      exit_event_loop();
       return false;
     }
 
-    const auto ready = api::AsyncTask::poll(tasks);
-    for (auto index : ready) {
+    auto ready = api::AsyncTask::poll(tasks);
+    const auto ready_len = ready.size();
+    // ensure ready list is monotonic
+    if (!std::is_sorted(ready.begin(), ready.end())) {
+      std::sort(ready.begin(), ready.end());
+    }
+    for (auto i = 0; i < ready_len; i++) {
+      // index is offset by previous erasures
+      auto index = ready.at(i) - i;
       auto task = tasks->at(index);
-      if (!task->run(engine)) {
+      bool success = task->run(engine);
+      tasks->erase(tasks->begin() + index);
+      if (!success) {
+        exit_event_loop();
         return false;
       }
-      tasks->erase(tasks->begin() + index);
-      if (lifetime_complete())
+      js::RunJobs(cx);
+      if (JS_IsExceptionPending(cx)) {
+        exit_event_loop();
+        return false;
+      }
+      if (interest_complete()) {
+        exit_event_loop();
         return true;
+      }
     }
   }
-
-  return true;
 }
 
 void EventLoop::init(JSContext *cx) { queue.init(cx); }

runtime/event_loop.h

@@ -25,12 +25,13 @@ class EventLoop {
   static bool has_pending_async_tasks();
 
   /**
-   * Run the event loop until all lifetimes are complete
+   * Run the event loop until all interests are complete.
+   * See run_event_loop in extension-api.h for the complete description.
    */
   static bool run_event_loop(api::Engine *engine, double total_compute);
 
-  static void incr_event_loop_lifetime();
-  static void decr_event_loop_lifetime();
+  static void incr_event_loop_interest();
+  static void decr_event_loop_interest();
 
   /**
    * Select on the next async tasks

tests/tests.cmake

@@ -8,6 +8,7 @@ function(test_e2e TEST_NAME)
     get_target_property(RUNTIME_DIR starling.wasm BINARY_DIR)
     add_test(${TEST_NAME} ${BASH_PROGRAM} ${CMAKE_SOURCE_DIR}/tests/test.sh ${RUNTIME_DIR} ${CMAKE_SOURCE_DIR}/tests/e2e/${TEST_NAME})
     set_property(TEST ${TEST_NAME} PROPERTY ENVIRONMENT "WASMTIME=${WASMTIME};WIZER=${WIZER_DIR}/wizer;WASM_TOOLS=${WASM_TOOLS_DIR}/wasm-tools")
+    set_tests_properties(${TEST_NAME} PROPERTIES TIMEOUT 30)
 endfunction()
 
 add_custom_target(integration-test-server DEPENDS test-server.wasm)
@@ -25,6 +26,7 @@ function(test_integration TEST_NAME)
 
     add_test(${TEST_NAME} ${BASH_PROGRAM} ${CMAKE_SOURCE_DIR}/tests/test.sh ${RUNTIME_DIR} ${CMAKE_SOURCE_DIR}/tests/integration/${TEST_NAME} ${RUNTIME_DIR}/test-server.wasm ${TEST_NAME})
     set_property(TEST ${TEST_NAME} PROPERTY ENVIRONMENT "WASMTIME=${WASMTIME};WIZER=${WIZER_DIR}/wizer;WASM_TOOLS=${WASM_TOOLS_DIR}/wasm-tools")
+    set_tests_properties(${TEST_NAME} PROPERTIES TIMEOUT 30)
 endfunction()
 
 test_e2e(smoke)