DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
434 stars 59 forks source link

`AsyncRuntime::idle()` makes futures !Send #276

Closed shouya closed 2 months ago

shouya commented 4 months ago

Hi! I have this use case where I want a runtime to be shared across thread and execute code at my wish. However, I find that I'm prevented from running AsyncRuntime::idle() or AsyncRuntime::execute_pending_job() in an async context where the future needs to implement Send.

Here's a minimal example to produce the error:

use rquickjs::prelude::*;

struct MyRuntime {
  context: rquickjs::AsyncContext,
}

impl MyRuntime {
  async fn new() -> Self {
    let runtime = rquickjs::AsyncRuntime::new().unwrap();
    let context = rquickjs::AsyncContext::full(&runtime).await.unwrap();
    Self { context }
  }

  async fn run<V>(&self, code: &str) -> Result<V, rquickjs::Error>
  where
    V: for<'js> FromJs<'js> + Send + 'static,
  {
    let out = self.context.with(|ctx| ctx.eval(code)).await?;
    self.context.runtime().idle().await; // Removing this line makes the future Send
    out
  }
}

#[tokio::main]
async fn main() {
  let runtime = MyRuntime::new().await;
  let task = tokio::task::spawn(async move {
    let res: i32 = runtime.run("1 + 2").await.unwrap();
    assert_eq!(res, 3);
  });

  task.await.unwrap();
}

And here's the compiler output:

/home/shou/.cargo/bin/cargo run --manifest-path /home/shou/tmp/rquickjs/foo/Cargo.toml  
   Compiling foo v0.1.0 (/home/shou/tmp/rquickjs/foo)
error[E0277]: `RefCell<Vec<Option<Pin<Box<dyn Future<Output = ()>>>>>>` cannot be shared between threads safely
   --> src/main.rs:27:14
    |
27  |   let task = tokio::task::spawn(async move {
    |              ^^^^^^^^^^^^^^^^^^ `RefCell<Vec<Option<Pin<Box<dyn Future<Output = ()>>>>>>` cannot be shared between threads safely
    |
    = help: within `rquickjs::runtime::spawner::Spawner<'_>`, the trait `Sync` is not implemented for `RefCell<Vec<Option<Pin<Box<dyn Future<Output = ()>>>>>>`
    = note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` instead
note: required because it appears within the type `rquickjs::runtime::spawner::Spawner<'_>`
   --> /home/shou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rquickjs-core-0.5.1/src/runtime/spawner.rs:20:12
    |
20  | pub struct Spawner<'js> {
    |            ^^^^^^^
    = note: required for `&rquickjs::runtime::spawner::Spawner<'_>` to implement `Send`
note: required because it appears within the type `rquickjs::runtime::spawner::SpawnFuture<'_, '_>`
   --> /home/shou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rquickjs-core-0.5.1/src/runtime/spawner.rs:61:12
    |
61  | pub struct SpawnFuture<'a, 'js>(&'a Spawner<'js>);
    |            ^^^^^^^^^^^
    = note: required because it captures the following types: `&AsyncRuntime`, `async_lock::mutex::MutexGuard<'_, rquickjs::runtime::r#async::InnerRuntime>`, `async_lock::mutex::Lock<'_, rquickjs::runtime::r#async::InnerRuntime>`, `Result<bool, rquickjs_core::result::AsyncJobException>`, `rquickjs::runtime::spawner::SpawnFuture<'_, '_>`
note: required because it's used within this `async` fn body
   --> /home/shou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rquickjs-core-0.5.1/src/runtime/async.rs:283:30
    |
283 |       pub async fn idle(&self) {
    |  ______________________________^
284 | |         let mut lock = self.inner.lock().await;
285 | |         lock.runtime.update_stack_top();
286 | |         lock.drop_pending();
...   |
319 | |         }
320 | |     }
    | |_____^
    = note: required because it captures the following types: `&MyRuntime`, `&str`, `Result<i32, rquickjs::Error>`, `impl Future<Output = Result<Result<i32, rquickjs::Error>, rquickjs::Error>>`, `impl Future<Output = ()>`
note: required because it's used within this `async` fn body
   --> src/main.rs:17:3
    |
17  | /   {
18  | |     let out = self.context.with(|ctx| ctx.eval(code)).await?;
19  | |     self.context.runtime().idle().await;
20  | |     out
21  | |   }
    | |___^
    = note: required because it captures the following types: `impl Future<Output = Result<i32, rquickjs::Error>>`
note: required because it's used within this `async` block
   --> src/main.rs:27:33
    |
27  |     let task = tokio::task::spawn(async move {
    |  _________________________________^
28  | |     let res: i32 = runtime.run("1 + 2").await.unwrap();
29  | |     assert_eq!(res, 3);
30  | |   });
    | |___^
note: required by a bound in `tokio::spawn`
   --> /home/shou/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/task/spawn.rs:166:21
    |
164 |     pub fn spawn<F>(future: F) -> JoinHandle<F::Output>
    |            ----- required by a bound in this function
165 |     where
166 |         F: Future + Send + 'static,
    |                     ^^^^ required by this bound in `spawn`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `foo` (bin "foo") due to 1 previous error

And the dependencies just for completeness:

[dependencies]
rquickjs = { version = "0.5.1", features = ["futures", "parallel"] }
tokio = { version = "1.36.0", features = ["macros", "rt-multi-thread"] }

The problem was because that the Spawner is !Send because it uses an RefCell internally, which is !Send:

// rquickjs-core-0.5.1/src/runtime/spawner.rs:20:12
type FuturesVec<T> = RefCell<Vec<Option<T>>>;

/// A structure to hold futures spawned inside the runtime.
///
/// TODO: change future lookup in poll from O(n) to O(1).
pub struct Spawner<'js> {
    futures: FuturesVec<Pin<Box<dyn Future<Output = ()> + 'js>>>,
    wakeup: Vec<Waker>,
}

I'm not very sure about how async is implemented in rquickjs. From a glimpse I think this may be solved by replacing the RefCell with a Mutex or RwLock. I wonder if there is a reason for using RefCell here?

Perhaps I'm using the crate wrong here. My goal is to allow both the runtime and the run function to be dispatch across thread boundaries while ensuring all jobs run to a completion. Please correct me if I'm doing it wrong. Thanks!