denoland / rusty_v8

Rust bindings for the V8 JavaScript engine
https://crates.io/crates/v8
MIT License
3.39k stars 315 forks source link

`v8::OwnedIsolate` instances must be dropped in the reverse order of creation. #1628

Closed kckeiks closed 1 month ago

kckeiks commented 1 month ago

Hi,

We have an app that is more or less doing the following:

Pseudo code:

fn main() {
    JsRuntime::init_platform(None);

    let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel::<IsolateHandle>();
    tokio::spawn(async move {
        while let Some(handle) = rx.recv().await {
            tokio::spawn(async move {
                tokio::time::sleep(params::REQ_TIMEOUT).await;
                handle.terminate_execution();
            });
        }
    });

    let mut pool = Vec::new();
    for _ in 0..THREAD_COUNT {
        let (job_tx, mut job_rx) = tokio::sync::mpsc::channel(1);
        pool.push(job_tx);

        std::thread::spawn(move || {
            tokio::runtime::Builder::new_current_thread()
                .enable_all()
                .build()
                .expect("failed to create connection async runtime")
                .block_on(async move {
                    let local = tokio::task::LocalSet::new();
                    local
                        .run_until(async move {
                            while let Some((tx, conn)) = job_rx.recv().await {
                                tokio::task::spawn_local(async move {
                                    if let Err(e) = handle_connection(tx, conn).await {
                                        error!("session failed: {e:?}");
                                    }
                                });
                            }
                        })
                        .await;
                });
        });
    }

    while let Ok(conn) = listener.accept().await {
        let tx = tx.clone();
        let mut job_params = Some((tx, conn));
        'outer: loop {
            for job_tx in &pool {
                let (tx, conn) = job_params.unwrap();
                match job_tx.try_send((tx, conn)) {
                    Ok(_) => break 'outer,
                    Err(TrySendError::Full((tx, conn))) => job_params = Some((tx, conn)),
                    Err(TrySendError::Closed((tx, conn))) => {
                        job_params = Some((tx, conn));
                    },
                };
            }
        }
    }
}

fn handle_connection(
    tx: UnboundedSender<IsolateHandle>,
   _ conn: (),
) -> Result<()> {
   // ..
    let mut runtime =
        Runtime::new(location.clone(), depth).context("Failed to initialize runtime")?;
    tx.send(runtime.deno.v8_isolate().thread_safe_handle())
        .context("Failed to send the IsolateHandle to main thread.")?;

    // ..
    let id = self.deno.load_main_es_module(specifier).await?;
    self.deno
        .run_event_loop(PollEventLoopOptions::default())
        .await?;
    self.deno.mod_evaluate(id).await?;

    let main = self.deno.get_module_namespace(id)?;
    let scope = &mut self.deno.handle_scope();
    let scope = &mut v8::TryCatch::new(scope);
    let main_local = v8::Local::new(scope, main);
    // ..
    let main_str = v8::String::new_external_onebyte_static(scope, b"main").unwrap();
    let main_fn = main_local.get(scope, main_str.into()).unwrap();
    // ..
    let main_fn = v8::Local::<v8::Function>::try_from(main_fn)?;
    main_fn.call(scope, undefined.into(), &[param]) 
    // ..
}

When we send multiple requests in parallel to our app, we hit the assertion v8::OwnedIsolate instances must be dropped in the reverse order of creation. Could you please advise on how we could be causing isolates to drop out of order? Do you have any suggestions on how to better use your API to solve our problem?

Thanks in advance.

devsnek commented 1 month ago

V8 contains a thread-local variable referring to the "current" isolate. When one handle_connection task suspends, another one can run, but the current isolate thread-local variable is not updated. You can call Isolate::enter/exit to update this variable.

deno_core, which it looks like you are using, is probably going to struggle in this regard because it assumes one runtime per thread, and so it doesn't include calls to handle this enter/exit stuff.