From f8dea1b31ca3b16ab4ac8a5ddc86d4e0d8d8b53b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= Date: Mon, 8 Jul 2024 15:10:27 -0400 Subject: [PATCH] Tidy up event loop handling code (#697) * Tidy up event loop handling code This commit refactors the event loop handling code, mainly by extracting the common pieces into a reusable function. Additionally, this commit ensures that the result of `Promise.finish` is corectly handled, which fixes execution of code with top level awaits. Finally, this commit also ensures that we test the `experimental_event_loop` in CI for the CLI and core crates (follow-up to https://github.com/bytecodealliance/javy/pull/238) * Clippy fixes * Assert fuel conditionally When the `experimental_event_loop` feature is enabled, a bit more work is done when executing JS code, therefore there's more fuel usage reported in some tests. * Create `cli-features.yml` * Add right naming * Fix `runs-on` key * Typo and fixes --- .github/workflows/cli-features.yml | 23 +++++++ crates/cli/tests/integration_test.rs | 14 ++++ .../tests/sample-scripts/top-level-await.js | 6 ++ crates/core/src/execution.rs | 66 ++++++++++--------- 4 files changed, 77 insertions(+), 32 deletions(-) create mode 100644 .github/workflows/cli-features.yml create mode 100644 crates/cli/tests/sample-scripts/top-level-await.js diff --git a/.github/workflows/cli-features.yml b/.github/workflows/cli-features.yml new file mode 100644 index 00000000..9c8cfe27 --- /dev/null +++ b/.github/workflows/cli-features.yml @@ -0,0 +1,23 @@ +# Tests extra CLI features and their dependency with core features. +name: Test CLI Features +on: + push: + branches: + - main + pull_request: + +jobs: + cli: + name: Test CLI Features + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: ./.github/actions/ci-shared-setup + with: + os: linux + + - name: Test `experimental_event_loop` + run: | + cargo build --package=javy-core --target=wasm32-wasi --release --features=experimental_event_loop + cargo test --package=javy-cli --features=experimental_event_loop --release -- --nocapture diff --git a/crates/cli/tests/integration_test.rs b/crates/cli/tests/integration_test.rs index f3573499..ab8e85ac 100644 --- a/crates/cli/tests/integration_test.rs +++ b/crates/cli/tests/integration_test.rs @@ -147,6 +147,20 @@ fn test_promises() -> Result<()> { Ok(()) } +#[cfg(feature = "experimental_event_loop")] +#[test] +fn test_promise_top_level_await() -> Result<()> { + let mut runner = Builder::default() + .bin(BIN) + .root(sample_scripts()) + .input("top-level-await.js") + .build()?; + let (out, _, _) = run(&mut runner, &[]); + + assert_eq!("bar", String::from_utf8(out)?); + Ok(()) +} + #[test] fn test_exported_functions() -> Result<()> { let mut runner = Builder::default() diff --git a/crates/cli/tests/sample-scripts/top-level-await.js b/crates/cli/tests/sample-scripts/top-level-await.js new file mode 100644 index 00000000..e39a83fb --- /dev/null +++ b/crates/cli/tests/sample-scripts/top-level-await.js @@ -0,0 +1,6 @@ +async function foo() { + return Promise.resolve("bar"); +} + +const output = new TextEncoder().encode(await foo()); +Javy.IO.writeSync(1, output); diff --git a/crates/core/src/execution.rs b/crates/core/src/execution.rs index bf73dfb0..459a7d37 100644 --- a/crates/core/src/execution.rs +++ b/crates/core/src/execution.rs @@ -3,7 +3,7 @@ use std::process; use anyhow::{anyhow, bail, Error, Result}; use javy::{ from_js_error, - quickjs::{context::EvalOptions, Module, Value}, + quickjs::{context::EvalOptions, Ctx, Error as JSError, Module, Value}, to_js_error, Runtime, }; @@ -24,21 +24,10 @@ pub fn run_bytecode(runtime: &Runtime, bytecode: &[u8]) { let module = unsafe { Module::load(this.clone(), bytecode)? }; let (_, promise) = module.eval()?; - if cfg!(feature = "experimental_event_loop") { - // If the experimental event loop is enabled, trigger it. - promise.finish::().map(|_| ()) - } else { - // Else we simply expect the promise to resolve immediately. - match promise.result() { - None => Err(to_js_error(this, anyhow!(EVENT_LOOP_ERR))), - Some(r) => r, - } - } + handle_maybe_promise(this.clone(), promise.into()) }) .map_err(|e| runtime.context().with(|cx| from_js_error(cx.clone(), e))) - // Prefer calling `process_event_loop` *outside* of the `with` callback, - // to avoid errors regarding multiple mutable borrows. - .and_then(|_| process_event_loop(runtime)) + .and_then(|_: ()| ensure_pending_jobs(runtime)) .unwrap_or_else(handle_error) } @@ -63,34 +52,47 @@ pub fn invoke_function(runtime: &Runtime, fn_module: &str, fn_name: &str) { opts.global = false; let value = this.eval_with_options::, _>(js, opts)?; - if let Some(promise) = value.as_promise() { - if cfg!(feature = "experimental_event_loop") { - // If the experimental event loop is enabled, trigger it. - promise.finish::().map(|_| ()) + handle_maybe_promise(this.clone(), value) + }) + .map_err(|e| runtime.context().with(|cx| from_js_error(cx.clone(), e))) + .and_then(|_: ()| ensure_pending_jobs(runtime)) + .unwrap_or_else(handle_error) +} + +/// Handles the promise returned by evaluating the JS bytecode. +fn handle_maybe_promise(this: Ctx, value: Value) -> javy::quickjs::Result<()> { + match value.as_promise() { + Some(promise) => { + if cfg!(feature = "experimental_event_loop") { + // If the experimental event loop is enabled, trigger it. + let resolved = promise.finish::(); + // `Promise::finish` returns Err(Wouldblock) when the all + // pending jobs have been handled. + if let Err(JSError::WouldBlock) = resolved { + Ok(()) } else { - // Else we simply expect the promise to resolve immediately. - match promise.result() { - None => Err(to_js_error(this, anyhow!(EVENT_LOOP_ERR))), - Some(r) => r, - } + resolved.map(|_| ()) } } else { - Ok(()) + // Else we simply expect the promise to resolve immediately. + match promise.result() { + None => Err(to_js_error(this, anyhow!(EVENT_LOOP_ERR))), + Some(r) => r, + } } - }) - .map_err(|e| runtime.context().with(|cx| from_js_error(cx.clone(), e))) - .and_then(|_: ()| process_event_loop(runtime)) - .unwrap_or_else(handle_error) + } + None => Ok(()), + } } -fn process_event_loop(rt: &Runtime) -> Result<()> { +fn ensure_pending_jobs(rt: &Runtime) -> Result<()> { if cfg!(feature = "experimental_event_loop") { - rt.resolve_pending_jobs()? + rt.resolve_pending_jobs() } else if rt.has_pending_jobs() { bail!(EVENT_LOOP_ERR); + } else { + Ok(()) } - - Ok(()) } fn handle_error(e: Error) {