DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Graceful shutdown of async executor #130

Closed stevefan1999-personal closed 1 year ago

stevefan1999-personal commented 1 year ago

I want to shutdown the executor when my script ends, but right now it doesn't seems like it is working.

Given that I have these error on Ctrl+C

JoinError::Cancelled(Id(19))
JoinError::Cancelled(Id(20))
The application panicked (crashed).
Message:  Async executor unfortunately destroyed: "SendError(..)"
Location: C:\Users\steve\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rquickjs-core-0.1.7\src\runtime\async_executor.rs:110

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 14 frames hidden ⋮
  15: enum2$<core::result::Result<tuple$<>,flume::SendError<async_task::runnable::Runnable<tuple$<> > > > >::expect<tuple$<>,flume::SendError<async_task::runnable::Runnable<tuple$<> > > ><unknown>
      at /rustc/5e1d3299a290026b85787bc9c7e72bcc53ac283f\library\core\src\result.rs:1049
  16: rquickjs_core::runtime::async_executor::impl$2::schedule::closure$0<unknown>
      at C:\Users\steve\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rquickjs-core-0.1.7\src\runtime\async_executor.rs:108
  17: async_task::runnable::impl$2::schedule<tuple$<>,rquickjs_core::runtime::async_executor::impl$2::schedule::closure_env$0><unknown>
      at C:\Users\steve\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\async-task-4.4.0\src\runnable.rs:108
  18: async_task::raw::RawTask<enum2$<rquickjs_core::runtime::async_executor::impl$2::spawn::async_block_env$0<enum2$<rquickjs_fun::js::timer::set_interval::async_block_env$0> > >,tuple$<>,rquickjs_core::runtime::async_executor::impl$2::schedule::closure_env$0,<unknown>
      at C:\Users\steve\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\async-task-4.4.0\src\raw.rs:352
  19: async_task::raw::RawTask<enum2$<rquickjs_core::runtime::async_executor::impl$2::spawn::async_block_env$0<enum2$<rquickjs_fun::js::timer::set_interval::async_block_env$0> > >,tuple$<>,rquickjs_core::runtime::async_executor::impl$2::schedule::closure_env$0,<unknown>
      at C:\Users\steve\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\async-task-4.4.0\src\raw.rs:236

It seems like the error comes from here: https://github.com/DelSkayn/rquickjs/blob/234cc91ff7a3d06fb5d58fcd8f0e6bed56af51d5/core/src/runtime/async_executor.rs#L106-L113

I simply just break from the loop in my main Tokio process, and if I do await for the executor handle it will run forever which is not what I wanted after Ctrl+C. I wonder what is the best practice to deal with this.

I'm using the parallel feature and I think we will need a task abort mechanism which is likely one of these:

  1. Cancellation token. For each spawned task in ctx, attach a global cancellation token, and when it was triggered, cancel all tasks.
  2. Manually add a cancellation signal for each futures you spawned in ctx. This is similar to cancellation token although it is not mandatory
  3. Use panic abort although this is more of a hail-marry approach (because you will lost valuable debug information for this)
  4. Migrate the executor task to a specific thread, and stop accepting future works. I think this would be the most likely solution as it should be easily implementable.
DelSkayn commented 1 year ago

Hi!

I am currently redesigning how futures are used in rquickjs in the PR #156. With this redesign we no longer use tasks to schedule futures spawned by rquickjs.

This should fix your issue.