Propagate jobs into JobExecutor::run_jobs

boa-dev · Jan 14, 2025 · 9c35fcd · 9c35fcd
1 parent a7c10d3
commit 9c35fcd
Show file tree

Hide file tree

Showing 19 changed files with 125 additions and 100 deletions.
diff --git a/cli/src/main.rs b/cli/src/main.rs
@@ -18,7 +18,7 @@ use boa_engine::{
     optimizer::OptimizerOptions,
     script::Script,
     vm::flowgraph::{Direction, Graph},
-    Context, JsError, Source,
+    Context, JsError, JsResult, Source,
 };
 use boa_parser::source::ReadChar;
 use clap::{Parser, ValueEnum, ValueHint};
@@ -292,7 +292,7 @@ fn evaluate_file(
         );
 
         let promise = module.load_link_evaluate(context);
-        context.run_jobs();
+        context.run_jobs().map_err(|err| err.into_erased(context))?;
         let result = promise.state();
 
         return match result {
@@ -308,9 +308,9 @@ fn evaluate_file(
         Ok(v) => println!("{}", v.display()),
         Err(v) => eprintln!("Uncaught {v}"),
     }
-    context.run_jobs();
-
-    Ok(())
+    context
+        .run_jobs()
+        .map_err(|err| err.into_erased(context).into())
 }
 
 fn evaluate_files(args: &Opt, context: &mut Context, loader: &SimpleModuleLoader) {
@@ -425,7 +425,9 @@ fn main() -> Result<()> {
                             eprintln!("{}: {}", "Uncaught".red(), v.to_string().red());
                         }
                     }
-                    context.run_jobs();
+                    if let Err(err) = context.run_jobs() {
+                        eprintln!("{err}");
+                    };
                 }
             }
 
@@ -467,10 +469,10 @@ impl JobExecutor for Executor {
         }
     }
 
-    fn run_jobs(&self, context: &mut Context) {
+    fn run_jobs(&self, context: &mut Context) -> JsResult<()> {
         loop {
             if self.promise_jobs.borrow().is_empty() && self.async_jobs.borrow().is_empty() {
-                return;
+                return Ok(());
             }
 
             let jobs = std::mem::take(&mut *self.promise_jobs.borrow_mut());

diff --git a/core/engine/src/builtins/promise/tests.rs b/core/engine/src/builtins/promise/tests.rs
@@ -13,8 +13,7 @@ fn promise() {
                     count += 1;
                 "#}),
         TestAction::assert_eq("count", 2),
-        #[allow(clippy::redundant_closure_for_method_calls)]
-        TestAction::inspect_context(|ctx| ctx.run_jobs()),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert_eq("count", 3),
     ]);
 }
diff --git a/core/engine/src/context/mod.rs b/core/engine/src/context/mod.rs
@@ -477,9 +477,10 @@ impl Context {
 
     /// Runs all the jobs with the provided job executor.
     #[inline]
-    pub fn run_jobs(&mut self) {
-        self.job_executor().run_jobs(self);
+    pub fn run_jobs(&mut self) -> JsResult<()> {
+        let result = self.job_executor().run_jobs(self);
         self.clear_kept_objects();
+        result
     }
 
     /// Asynchronously runs all the jobs with the provided job executor.
@@ -490,11 +491,13 @@ impl Context {
     /// specific handling of each [`JobExecutor`]. If you want to execute jobs concurrently, you must
     /// provide a custom implementatin of `JobExecutor` to the context.
     #[allow(clippy::future_not_send)]
-    pub async fn run_jobs_async(&mut self) {
-        self.job_executor()
+    pub async fn run_jobs_async(&mut self) -> JsResult<()> {
+        let result = self
+            .job_executor()
             .run_jobs_async(&RefCell::new(self))
             .await;
         self.clear_kept_objects();
+        result
     }
 
     /// Abstract operation [`ClearKeptObjects`][clear].

diff --git a/core/engine/src/job.rs b/core/engine/src/job.rs
@@ -386,7 +386,7 @@ pub trait JobExecutor {
     fn enqueue_job(&self, job: Job, context: &mut Context);
 
     /// Runs all jobs in the executor.
-    fn run_jobs(&self, context: &mut Context);
+    fn run_jobs(&self, context: &mut Context) -> JsResult<()>;
 
     /// Asynchronously runs all jobs in the executor.
     ///
@@ -395,7 +395,7 @@ pub trait JobExecutor {
     fn run_jobs_async<'a, 'b, 'fut>(
         &'a self,
         context: &'b RefCell<&mut Context>,
-    ) -> Pin<Box<dyn Future<Output = ()> + 'fut>>
+    ) -> Pin<Box<dyn Future<Output = JsResult<()>> + 'fut>>
     where
         'a: 'fut,
         'b: 'fut,
@@ -427,7 +427,9 @@ pub struct IdleJobExecutor;
 impl JobExecutor for IdleJobExecutor {
     fn enqueue_job(&self, _: Job, _: &mut Context) {}
 
-    fn run_jobs(&self, _: &mut Context) {}
+    fn run_jobs(&self, _: &mut Context) -> JsResult<()> {
+        Ok(())
+    }
 }
 
 /// A simple FIFO executor that bails on the first error.
@@ -438,7 +440,7 @@ impl JobExecutor for IdleJobExecutor {
 /// To disable running promise jobs on the engine, see [`IdleJobExecutor`].
 #[derive(Default)]
 pub struct SimpleJobExecutor {
-    jobs: RefCell<VecDeque<PromiseJob>>,
+    promise_jobs: RefCell<VecDeque<PromiseJob>>,
     async_jobs: RefCell<VecDeque<NativeAsyncJob>>,
 }
 
@@ -459,36 +461,38 @@ impl SimpleJobExecutor {
 impl JobExecutor for SimpleJobExecutor {
     fn enqueue_job(&self, job: Job, _: &mut Context) {
         match job {
-            Job::PromiseJob(p) => self.jobs.borrow_mut().push_back(p),
+            Job::PromiseJob(p) => self.promise_jobs.borrow_mut().push_back(p),
             Job::AsyncJob(a) => self.async_jobs.borrow_mut().push_back(a),
         }
     }
 
-    fn run_jobs(&self, context: &mut Context) {
+    fn run_jobs(&self, context: &mut Context) -> JsResult<()> {
         let context = RefCell::new(context);
         loop {
             let mut next_job = self.async_jobs.borrow_mut().pop_front();
             while let Some(job) = next_job {
-                if pollster::block_on(job.call(&context)).is_err() {
+                if let Err(err) = pollster::block_on(job.call(&context)) {
                     self.async_jobs.borrow_mut().clear();
-                    return;
+                    self.promise_jobs.borrow_mut().clear();
+                    return Err(err);
                 };
                 next_job = self.async_jobs.borrow_mut().pop_front();
             }
 
             // Yeah, I have no idea why Rust extends the lifetime of a `RefCell` that should be immediately
             // dropped after calling `pop_front`.
-            let mut next_job = self.jobs.borrow_mut().pop_front();
+            let mut next_job = self.promise_jobs.borrow_mut().pop_front();
             while let Some(job) = next_job {
-                if job.call(&mut context.borrow_mut()).is_err() {
-                    self.jobs.borrow_mut().clear();
-                    return;
+                if let Err(err) = job.call(&mut context.borrow_mut()) {
+                    self.async_jobs.borrow_mut().clear();
+                    self.promise_jobs.borrow_mut().clear();
+                    return Err(err);
                 };
-                next_job = self.jobs.borrow_mut().pop_front();
+                next_job = self.promise_jobs.borrow_mut().pop_front();
             }
 
-            if self.async_jobs.borrow().is_empty() && self.jobs.borrow().is_empty() {
-                return;
+            if self.async_jobs.borrow().is_empty() && self.promise_jobs.borrow().is_empty() {
+                return Ok(());
             }
         }
     }

diff --git a/core/engine/src/object/builtins/jspromise.rs b/core/engine/src/object/builtins/jspromise.rs
@@ -1143,14 +1143,14 @@ impl JsPromise {
     /// // Uncommenting the following line would panic.
     /// // context.run_jobs();
     /// ```
-    pub fn await_blocking(&self, context: &mut Context) -> Result<JsValue, JsValue> {
+    pub fn await_blocking(&self, context: &mut Context) -> Result<JsValue, JsError> {
         loop {
             match self.state() {
                 PromiseState::Pending => {
-                    context.run_jobs();
+                    context.run_jobs()?;
                 }
                 PromiseState::Fulfilled(f) => break Ok(f),
-                PromiseState::Rejected(r) => break Err(r),
+                PromiseState::Rejected(r) => break Err(JsError::from_opaque(r)),
             }
         }
     }

diff --git a/core/engine/src/tests/async_generator.rs b/core/engine/src/tests/async_generator.rs
@@ -47,7 +47,7 @@ fn return_on_then_infinite_loop() {
             });
             g.return();
         "#}),
-        TestAction::inspect_context(Context::run_jobs),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert_eq("count", 100),
     ]);
 }
@@ -71,7 +71,7 @@ fn return_on_then_single() {
             });
             let ret = g.return()
         "#}),
-        TestAction::inspect_context(Context::run_jobs),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert_eq("first", false),
         TestAction::assert_with_op("ret", |ret, context| {
             assert_promise_iter_value(&ret, &JsValue::undefined(), true, context);
@@ -104,7 +104,7 @@ fn return_on_then_queue() {
             let second = g.next();
             let ret = g.return();
         "#}),
-        TestAction::inspect_context(Context::run_jobs),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert_with_op("first", |first, context| {
             assert_promise_iter_value(&first, &JsValue::from(1), false, context);
             true

diff --git a/core/engine/src/tests/iterators.rs b/core/engine/src/tests/iterators.rs
@@ -46,8 +46,7 @@ fn iterator_close_in_continue_before_jobs() {
                 actual.push("async fn end");
             }();
         "#}),
-        #[allow(clippy::redundant_closure_for_method_calls)]
-        TestAction::inspect_context(|ctx| ctx.run_jobs()),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert(indoc! {r#"
             arrayEquals(
                 actual,
@@ -110,8 +109,7 @@ fn async_iterator_close_in_continue_is_awaited() {
                 actual.push("async fn end");
             }();
         "#}),
-        #[allow(clippy::redundant_closure_for_method_calls)]
-        TestAction::inspect_context(|ctx| ctx.run_jobs()),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert(indoc! {r#"
             arrayEquals(
                 actual,
@@ -198,8 +196,7 @@ fn mixed_iterators_close_in_continue() {
                 actual.push("async fn end");
             }();
         "#}),
-        #[allow(clippy::redundant_closure_for_method_calls)]
-        TestAction::inspect_context(|ctx| ctx.run_jobs()),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert(indoc! {r#"
             arrayEquals(
                 actual,

diff --git a/core/engine/src/tests/promise.rs b/core/engine/src/tests/promise.rs
@@ -31,7 +31,7 @@ fn issue_2658() {
                     genTwo.next().then(v => { result2 = v; });
                 "#
         }),
-        TestAction::inspect_context(|ctx| ctx.run_jobs()),
+        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
         TestAction::assert("!result1.done"),
         TestAction::assert_eq("result1.value", 5),
         TestAction::assert("!result2.done"),

diff --git a/core/engine/tests/imports.rs b/core/engine/tests/imports.rs
@@ -23,7 +23,7 @@ fn subdirectories() {
     let module = boa_engine::Module::parse(source, None, &mut context).unwrap();
     let result = module.load_link_evaluate(&mut context);
 
-    context.run_jobs();
+    context.run_jobs().unwrap();
     match result.state() {
         PromiseState::Pending => {}
         PromiseState::Fulfilled(v) => {

diff --git a/core/engine/tests/module.rs b/core/engine/tests/module.rs
@@ -41,7 +41,7 @@ fn test_json_module_from_str() {
 
     let module = Module::parse(source, None, &mut context).unwrap();
     let promise = module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     match promise.state() {
         PromiseState::Pending => {}

diff --git a/core/interop/src/lib.rs b/core/interop/src/lib.rs
@@ -565,7 +565,7 @@ fn into_js_module() {
     let root_module = Module::parse(source, None, &mut context).unwrap();
 
     let promise_result = root_module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     // Checking if the final promise didn't return an error.
     assert!(
@@ -617,7 +617,7 @@ fn can_throw_exception() {
     let root_module = Module::parse(source, None, &mut context).unwrap();
 
     let promise_result = root_module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     // Checking if the final promise didn't return an error.
     assert_eq!(
@@ -721,7 +721,7 @@ fn class() {
     let root_module = Module::parse(source, None, &mut context).unwrap();
 
     let promise_result = root_module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     // Checking if the final promise didn't return an error.
     assert!(

diff --git a/core/interop/src/macros.rs b/core/interop/src/macros.rs
@@ -525,7 +525,7 @@ fn js_class_test() {
     let root_module = Module::parse(source, None, &mut context).unwrap();
 
     let promise_result = root_module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     // Checking if the final promise didn't return an error.
     assert!(

diff --git a/core/interop/tests/embedded.rs b/core/interop/tests/embedded.rs
@@ -35,7 +35,7 @@ fn simple() {
     )
     .expect("failed to parse module");
     let promise = module.load_link_evaluate(&mut context);
-    context.run_jobs();
+    context.run_jobs().unwrap();
 
     match promise.state() {
         PromiseState::Fulfilled(value) => {

diff --git a/examples/src/bin/module_fetch_async.rs b/examples/src/bin/module_fetch_async.rs
@@ -124,7 +124,7 @@ fn main() -> JsResult<()> {
 
     // Important to call `Context::run_jobs`, or else all the futures and promises won't be
     // pushed forward by the job queue.
-    context.run_jobs();
+    context.run_jobs()?;
 
     match promise.state() {
         // Our job queue guarantees that all promises and futures are finished after returning
@@ -204,23 +204,23 @@ impl JobExecutor for Queue {
     }
 
     // While the sync flavor of `run_jobs` will block the current thread until all the jobs have finished...
-    fn run_jobs(&self, context: &mut Context) {
-        smol::block_on(smol::LocalExecutor::new().run(self.run_jobs_async(&RefCell::new(context))));
+    fn run_jobs(&self, context: &mut Context) -> JsResult<()> {
+        smol::block_on(smol::LocalExecutor::new().run(self.run_jobs_async(&RefCell::new(context))))
     }
 
     // ...the async flavor won't, which allows concurrent execution with external async tasks.
     fn run_jobs_async<'a, 'b, 'fut>(
         &'a self,
         context: &'b RefCell<&mut Context>,
-    ) -> Pin<Box<dyn Future<Output = ()> + 'fut>>
+    ) -> Pin<Box<dyn Future<Output = JsResult<()>> + 'fut>>
     where
         'a: 'fut,
         'b: 'fut,
     {
         Box::pin(async move {
             // Early return in case there were no jobs scheduled.
             if self.promise_jobs.borrow().is_empty() && self.async_jobs.borrow().is_empty() {
-                return;
+                return Ok(());
             }
             let mut group = FutureGroup::new();
             loop {
@@ -231,7 +231,7 @@ impl JobExecutor for Queue {
                 if self.promise_jobs.borrow().is_empty() {
                     let Some(result) = group.next().await else {
                         // Both queues are empty. We can exit.
-                        return;
+                        return Ok(());
                     };
 
                     if let Err(err) = result {

diff --git a/examples/src/bin/modules.rs b/examples/src/bin/modules.rs
@@ -87,7 +87,7 @@ fn main() -> Result<(), Box<dyn Error>> {
         );
 
     // Very important to push forward the job queue after queueing promises.
-    context.run_jobs();
+    context.run_jobs()?;
 
     // Checking if the final promise didn't return an error.
     match promise_result.state() {