Open TelephoneTan opened 3 months ago
This is quite right, it does not cancel jobs if the parent job of evaluate
cancels.
Using "NonCancellable" is a bit of overkill in my opinion. Also, this may break the cancellation semantics: if you cancel "evaluate", but it only cancels the async job, it will continue to execute your JS code, what if it starts another job or calls something that I want to stop?
In my opinion, if evaluate
gets canceled, we should stop everything. If you need to stop only the jobs but keep the JS code running, you can throw in the binding function and handle rejection in JS. It’s a bit cumbersome to implement now, so I would probably expose a function or the async jobs for the cancellation.
As the kotlin coroutines' document states, coroutine cancel is cooperate, you can not cancel something from outside and force it to stop, it will cause damage.
Kotlin coroutines code listens to the isActive
state of current coroutine, and returns as soon as possible if the coroutine has been cancelled.
This is similar in JS, asyncFunction will throw CancellationException when the coroutine is cancelled, and the JS code will continue to do its job, handling this exception, releasing/cleaning the resources, etc, and finally return as soon as possible.
If we want the sync JS code can be cancelled, the only way is to expose an "isActive" variable to the JS code, so that the JS code can listen to the cancel and respond to it. Discarding the JS code without executing it is like a "force stop" and it will break the logic and cause damage.
There is no "force stop" in kotlin coroutines, there is only "cooperated cancel", so as it should be in QuickJS and JS world.
I didn't see how canceling evaluate
but it does not stop the JS execution making the cancellation cooperative, on the contrary, it makes "evaluate()" harder to cancel.
IMO, canceling evaluate
is kind of like calling process.exit()
in Node, you should expect it to stop your execution. If you want to break the execution nicely you should better run the cleanup before the cancellation.
There is a difference, after you call process.exit()
, the whole process is destroyed and all variables and states in the process are not available any more.
But after you cancel the evaluate
, the QuickJS instance is still there, not destroyed. If you make some changes in the JS world in one previous evaluate
, these changes are visible to the next evaluate
, the JS state is remained. That's why I said if you force stop an evaluate
, it will leave the JS world in a strange middle state.
So cancelling evaluate
is not like calling process.exit()
, instead, quickJS.close()
plays the same role as process.exit()
.
Sure, cancel
is not cooperative now, if you want it to be cooperative, just expose an isActive
variable or something to the JS world, and listen to it in the JS side. This is also how kotlin coroutines do.
like this:
while(true || isActive){// loop infinitely until evaluate is cancelled
console.log('I am alive')
}
console.log('evaluate cancelled, I quit now')
As for asyncFunction invoke, asyncFunction will throw a CancellationException
(raised by kotlin coroutines), thus JS code can catch this exception and know it is time to quit.
This is how JS code "cooperate" with an evaluate-cancel.
So cancelling evaluate is not like calling process.exit(), instead, quickJS.close() plays the same role as process.exit().
Interesting point, this is also quite right, if you have multiple evaluate
calls that do not depend on each other. Suppose we use the runtime to execute a single script, there may be a series of evaluate
calls, but if one fails, you probably don't want to use the polluted runtime, this is also some kind of exit().
if you want it to be cooperative, just expose an isActive variable or something to the JS world, and listen to it in the JS side
This is weird, we should not put the responsibility of cancellation handling on JS code. When writing JS, I don't think anyone cares what happens if the runtime decides to cancel their code at some point.
Yes, they are two totally different use cases.
My use case is I implement part of my app logic using JS, so I can update the app logic remotely from server side, without requiring my users to update the app on their phone.
So I create and reuse a QuickJS instance globally, providing a lot of functions in JS, call evaluate
every time I need to invoke the logic writen in JS. My JS functions don't depend on any global state at all, so I don't need to worry the JS runtime is "polluted".
Using multiple QuickJS instances is also another solution, but I worry that the QuickJS instance is too heavy that creating a QuickJS instance every time will impact the performance.
This makes sense. As I always use it for executing one-shot scripts, so this library hasn't focused on long-live runtime.
I haven't benchmarked the runtime overhead, but if you're primarily concerned with time, on a 2019 high-end Android phone, "QuickJs.create()" will take about 5 milliseconds. I think that's acceptable for most non-critical use cases.
Besides QuickJS.create()
, I also has to eval a long webpacked javascript for every newly created QuickJS instance to register all JS functions in their runtime. That's horrible if I can't reuse the QuickJS instance.
Yeah, reusing is currently possible but getting it to work correctly and work well is kinda challenging, like something discussed in #108.
Currently, the
evaluate
can cooperate with coroutine cancel, it will return when the current coroutine is cancelled, this is good.BUT the async jobs triggered by
evaluate
including their callbacks will last forever and continue to run afterevaluate
has been cancelled, this is bad. We can say that the async jobs are leaked.The root cause is here:
invokeAsyncFunction
uses a independentCoroutineScope
instance to launch async jobs, but thisCoroutineScope
instance doesn't has the coroutineJob
ofevaluate
's current coroutine as its scopeJob
, so all async jobs escape out of the "structured concurrency" of theevaluate
's current coroutine and won't get cancelled ifevaluate
is cancelled.The solution is quite simple: inject the
Job
ofevaluate
that triggers async jobs to the coroutine scope of those async jobs, then every async job triggered by anevaluate
will listen to thisevaluate
'sJob
and get cancelled automatically when thisevaluate
is cancelled:Now our async jobs will get cancelled automatically, so we can wait for them in
evaluate
even if we are in cancelled state:Additionally, even when
evaluate
is cancelled, the sync JS code must continue to run to push the JS code state to the correct final state so we can make sure no pending jobs are leaked, and JS code is not in a strange middle state that could probably causes strange bugs. All the clean-up steps must run as usual too: