Some questions about cocurrency

[TelephoneTan]

I love this library, but I have some questions.

Question1: Can I use exactly one same QuickJS instance on two coroutines/threads?

Question2: According to the README, QuickJs.create(Dispatchers.Default), what exactly is the dispatcher parameter used for? Must it be a single-thread dispatcher(because JavaScript is a single-thread language)?

[dokar3]

Hi, thanks for using it!

For Question 1: You can share the instance between multiple threads, it's thread safe. But you won't able to evaluate concurrently, the instance uses some global states which prevents this, and quickjs itself doesn't seem to be thread-safe as far as I know.

For Question 2: The dispatcher passed in the create() function is the job dispatcher, which is used to execute async function bindings. It's safe to use a multi-threaded dispatcher since locks are used internally when interacting with the JS part.

[TelephoneTan]

Does every QuickJS instance hold an individual native quickjs instance under the hood which means they won't impact each other?

[TelephoneTan]

If I call function() twice with the same name, what would happen? Will the new function replace the old one has the same name? Will this cause a memory leak?

[TelephoneTan]

Hi, thanks for using it!

For Question 1: You can share the instance between multiple threads, it's thread safe. But you won't able to evaluate concurrently, the instance uses some global states which prevents this, and quickjs itself doesn't seem to be thread-safe as far as I know.

For Question 2: The dispatcher passed in the create() function is the job dispatcher, which is used to execute async function bindings. It's safe to use a multi-threaded dispatcher since locks are used internally when interacting with the JS part.

Is it ok to share a QuickJS instance between two different threads if both threads call evaluate sequentially(one after another, not concurrently)?

If the dispatcher is used to handle asyncFunction, then does the non async part JS code run directly on the thread that calls evaluate? Will this be a problem because two evaluate calls may lie on two different threads(even not concurrently, there can be memory-visibily issues between threads in JVM such as ThreadLocal variables)?

[dokar3]

Does every QuickJS instance hold an individual native quickjs instance under the hood which means they won't impact each other?

Yes.

If I call function() twice with the same name, what would happen? Will the new function replace the old one has the same name? Will this cause a memory leak?

Yes, the latest will be called when calling it from JS. There is no leak but the native data of the old function will remain until the instance is closed.

Is it ok to share a QuickJS instance between two different threads if both threads call evaluate sequentially(one after another, not concurrently)?

Yes, it's okay to do that. Concurrent calls should work too even though they can't be executed concurrently, but it does not work now, probably a bug, will fix this.

then does the non async part JS code run directly on the thread that calls evaluate?

Yes.

Will this be a problem because two evaluate calls may lie on two different threads(even not concurrently, there can be memory-visibily issues between threads in JVM such as ThreadLocal variables)?

Yes, async function bindings are called directly from CoroutineScope(jobDispatcher).launch {}, they are just like other suspend functions, so everything we had in the JVM world is still there.

[TelephoneTan]

Thank you for your quick reply.

Will this be a problem because two evaluate calls may lie on two different threads(even not concurrently, there can be memory-visibily issues between threads in JVM such as ThreadLocal variables)?

Yes, async function bindings are called directly from CoroutineScope(jobDispatcher).launch {}, they are just like other suspend functions, so everything we had in the JVM world is still there.

But I don't mean the asyncFunction call, I mean the evaluate, if the non async JS code run directly on the thread which makes the evaluate call, consider about this:

val js : QuickJS = TODO()
val lock : Lock = TODO()
GlobalScope.launch{
    lock.lock()
    js.evaluate("some code here")
    lock.unlock()
}
GlobalScope.launch{
    lock.lock()
    js.evaluate("some other code here")
    lock.unlock()
}

the two js.evaluate("some code here") here would never run concurrently because of the lock, and if the first evaluate to run make some normal memory state change under the hood, these changes will also be visible to the second evaluate, which guaranteed by the "happens before" relationship built by the lock, so there seems no thread-safe issues with the help of the lock.

BUT the lock won't help too if the evaluate call or the native code use ThreadLocal variables internally, because the two evaluate calls could be dispatched to two totally different threads.

so if the JS code executed by evaluate call directly runs on the same thread as the evaluate, the only way to eliminate the multithreading bugs is to make sure all evaluate calls are on the same thread, like this:

val js : QuickJS = TODO()
val singleThreadDispatcher : CoroutineDispatcher = TODO()
withContext(singleThreadDispatcher){ // <========== same thread
    js.evaluate("some code here")
}
withContext(singleThreadDispatcher){ // <========== same thread
    js.evaluate("some other code here")
}

Is this true? Please correct me if I am wrong.

[TelephoneTan]

By the way, I am really curious: If evaluate runs JS code directly on its current thread, what would happen if I evaluate JS code like this:

new Promise((resolve)=> {
    /** some IO code here ... **/
    resolve()
}).then(v=>{
    /** callback code here ... **/
})

Will the evaluate return immediately(because the code doesn't wait for the promise)? If so, will the Promise continue to do its job under the hood? If the Promise keep running, which thread does it run its callback code on?

[dokar3]

No problem.

Is this true? Please correct me if I am wrong.

Sorry for misunderstanding the problem. This is much more a quickjs question than a library question, as I know, quickjs does not use thread locals for anything, and neither does this library, so both multithreaded dispatchers and single-threaded dispatchers are okay in this case.

Will the evaluate return immediately(because the code doesn't wait for the promise)? If so, will the Promise continue to do its job under the hood?

Even if we don't await the promise, evaluate() will still be blocked until all jobs are completed.

If the Promise keep running, which thread does it run its callback code on?

In this case, its callback will be executed in the evaluate() thread. But if we await the IO call, the callback (or the subsequent code) will be executed in the thread used to run the IO code.

[TelephoneTan]

How can it be?

As far as I know, in JavaScript async/await is just a syntax sugar to simplify the traditional Promise.then() code style.

Why would its behaviour differ from the Promise.then() code？

[dokar3]

I have no idea how quickjs handle promises internally, but from the aspect of its API, if we await an external promise (asyncFunction in this library), the JS execution will be paused until resolve or reject is called from C code. This is something much like Kotlin Coroutines IMO.

In this library, resolve and reject are called (through JNI) in the coroutine used to run asyncFunction, so the subsequent execution is in the same thread.

new Promise(async (resolve) => {
  await delay(1000); // execution paused here
  resolve();
})
.then(() => {
  console.log("Done");
})

If we never await the promise created by asyncFunction and call resolve in the JS code, there is no pause, the callback code will be executed immediately.

new Promise(async (resolve) => {
  delay(1000);
  resolve();
})
.then(() => {
  console.log("Done");
})

[TelephoneTan]

That's wierd, could you please post some links to the relative quickjs api doc or something?

I can't find any description about the promise or await or event-loop on QuickJS's website. Sad.

[dokar3]

I also can't find any doc about quickjs' promise API, I haven't read the code but I believe it just follows the JS spec: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#description

[TelephoneTan]

I have no idea how quickjs handle promises internally, but from the aspect of its API, if we await an external promise (asyncFunction in this library), the JS execution will be paused until resolve or reject is called from C code. This is something much like Kotlin Coroutines IMO.

In this library, resolve and reject are called (through JNI) in the coroutine used to run asyncFunction, so the subsequent execution is in the same thread.

new Promise(async (resolve) => {
  await delay(1000); // execution paused here
  resolve();
})
.then(() => {
  console.log("Done");
})

If we never await the promise created by asyncFunction and call resolve in the JS code, there is no pause, the callback code will be executed immediately.

new Promise(async (resolve) => {
  delay(1000);
  resolve();
})
.then(() => {
  console.log("Done");
})

What do you mean by "the JS execution will be paused"?

Do you mean the whole js event loop being stopped, or just the code after the await statement being suspended?

If the whole js event loop stops, not only this Promise, but other Promise callbacks won't have a chance to run, and that's definitely not the standard behaviour documented on the MDN.

[dokar3]

or just the code after the await statement being suspended?

Yes.

Event loop does not come with the quickjs engine, instead we poll jobs manually in a loop. See https://github.com/dokar3/quickjs-kt/blob/1ef50decfed171dad6f477b89bdd696c092a279a/quickjs/src/jniMain/kotlin/com/dokar/quickjs/QuickJs.jni.kt#L347

[TelephoneTan]

I have check your code, I think they are just ok except here:

internal actual fun invokeAsyncFunction(
        args: Array<Any?>,
        block: suspend (bindingArgs: Array<Any?>) -> Any?,
    ) {
        ensureNotClosed()
        val (resolveHandle, rejectHandle) = promiseHandlesFromArgs(args)
        val job = coroutineScope.launch {
            try {
                val result = block(args.sliceArray(2..<args.size))
                jsMutex.withLock {
                    // Call resolve() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = resolveHandle,
                        args = arrayOf(result)
                    )
                }
            } catch (e: Throwable) {
                jsMutex.withLock {
                    // Call reject() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = rejectHandle,
                        args = arrayOf(e)
                    )
                }
            }
            jsMutex.withLock {
                while (executePendingJob(context, globals)) {
                    // The job is completed, see what we can do next
                }
            }
        }
        job.invokeOnCompletion {
            jobsMutex.withLockSync { asyncJobs -= job }
        }
        jobsMutex.withLockSync { asyncJobs += job }
    }

Why bother adding a while loop to executePendingJob after the async job finishes? There is already one loop to poll the pending task inside evaluate:

private suspend fun evalAndAwait(evalBlock: suspend () -> Any?): Any? {

ensureNotClosed()

evalException = null

loadModules()

jsMutex.withLock { evalBlock() }

awaitAsyncJobs()

val result = jsMutex.withLock { getEvaluateResult(context, globals) }

handleException()

return result

}

[dokar3]

It's for job cancellation when an async function throws and no one catches the error, we call it to fire an unhandled promise rejection call which then cancels all running jobs.

that might look weird but it's necessary because the 'event loop' we have in evaluate polls jobs and joins them all before starting the next poll so it won't cancel anything.

[TelephoneTan]

Interesting, could you point out the code that cancels jobs when asyncFunction fails?

[TelephoneTan]

I have check your code, I think they are just ok except here:

internal actual fun invokeAsyncFunction(
        args: Array<Any?>,
        block: suspend (bindingArgs: Array<Any?>) -> Any?,
    ) {
        ensureNotClosed()
        val (resolveHandle, rejectHandle) = promiseHandlesFromArgs(args)
        val job = coroutineScope.launch {
            try {
                val result = block(args.sliceArray(2..<args.size))
                jsMutex.withLock {
                    // Call resolve() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = resolveHandle,
                        args = arrayOf(result)
                    )
                }
            } catch (e: Throwable) {
                jsMutex.withLock {
                    // Call reject() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = rejectHandle,
                        args = arrayOf(e)
                    )
                }
            }
            jsMutex.withLock {
                while (executePendingJob(context, globals)) {
                    // The job is completed, see what we can do next
                }
            }
        }
        job.invokeOnCompletion {
            jobsMutex.withLockSync { asyncJobs -= job }
        }
        jobsMutex.withLockSync { asyncJobs += job }
    }

Why bother adding a while loop to executePendingJob after the async job finishes? There is already one loop to poll the pending task inside evaluate:

private suspend fun evalAndAwait(evalBlock: suspend () -> Any?): Any? {

ensureNotClosed()

evalException = null

loadModules()

jsMutex.withLock { evalBlock() }

awaitAsyncJobs()

val result = jsMutex.withLock { getEvaluateResult(context, globals) }

handleException()

return result

}

There is already a try...catch block around the async function invoke,

so how exactly does the while loop under the try...catch block help with "the error no one catches"? What error is it?

[dokar3]

You can find them in setUnhandledPromiseRejection(), which will be called by the C code.

executePendingJob() does the work of letting quickjs fire the call.

[TelephoneTan]

internal actual fun invokeAsyncFunction(
        args: Array<Any?>,
        block: suspend (bindingArgs: Array<Any?>) -> Any?,
    ) {
        ensureNotClosed()
        val (resolveHandle, rejectHandle) = promiseHandlesFromArgs(args)
        val job = coroutineScope.launch {
            try {
                val result = block(args.sliceArray(2..<args.size))
                jsMutex.withLock {
                    // Call resolve() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = resolveHandle,
                        args = arrayOf(result)
                    )
                }
            } catch (e: Throwable) {
                jsMutex.withLock {
                    // Call reject() on JNI side
                    invokeJsFunction(
                        context = context,
                        globals = globals,
                        handle = rejectHandle,
                        args = arrayOf(e)
                    )
                }
            }
            jsMutex.withLock {
                while (executePendingJob(context, globals)) {
                    // The job is completed, see what we can do next
                }
            }
        }
        job.invokeOnCompletion {
            jobsMutex.withLockSync { asyncJobs -= job }
        }
        jobsMutex.withLockSync { asyncJobs += job }
    }

If the job gets completed too quickly, the asyncJobs-=job will be executed before asyncJobd+=job, which will leak the job.

[TelephoneTan]

    private suspend fun awaitAsyncJobs() {
        /**
         * This is our simple 'event loop'.
         */
        while (true) {
            jsMutex.withLock {
                do {
                    val executed = executePendingJob(context, globals)
                } while (executed)
            }
            val jobs = jobsMutex.withLock { asyncJobs.filter { it.isActive } }
            if (jobs.isEmpty()) {
                // No jobs to run
                break
            }
            jobs.joinAll()
        }
    }

If the async kotlin code finishes too quickly, the resolve or reject has been called, the callback is executing by the executePendingJob in invokeAsyncFunction, but the async job has not been added to the asyncJobs, the evaluate call will return without waiting for invokeAsyncFunction'sexecutePendingJob.

[dokar3]

If the job gets completed too quickly, the asyncJobs-=job will be executed before asyncJobd+=job, which will leak the job.

Yep, this might happen but it will be fine even if we don't remove it from the list, jobs are not some resources from our perspective. Anyway, adjusting their order will hurt nothing.

the evaluate call will return without waiting for invokeAsyncFunction's executePendingJob.

I don't quite get this, executePendingJob is a synchronous call, how can it be that the job has been completed, but executePendingJob has not?

[TelephoneTan]

I don't quite get this, executePendingJob is a synchronous call, how can it be that the job has been completed, but executePendingJob has not?

Sorry, I was wrong, that is impossible.

BUT, I still have a question: if executePendingJob fires an unhandled promise rejection call, then why not leave the duty to the executePendingJob in awaitAsyncJobs, instead of adding an extra executePendingJob to take care of this in invokeAsyncFunction?

[dokar3]

I got it, awaitAsyncJobs can do this job. Suppose we have two async functions, one will fail and the other will not:

var isOkay = false

asyncFunction("fail") {
    delay(100)
    error("Failed")
}

asyncFunction("okay") {
   delay(500)
   isOkay = true   
}

evaluate<Any?>("okay(); fail();")

assertFalse(isOkay)

In invokeAsyncFunction we simply call executePendingJob() after the resolve/reject call, this will immediately cancel okay() after fail() throws;

In awaitAsyncJobs we might need to call invokeOnCompletion on every job so we can cancel okay() after fail() throws.

suspend fun awaitAsyncJobs() {
    ...
    jobs.forEach {
            it.invokeOnCompletion {
                while (executePendingJob(context, globals)) {}
            }
        }
    }
    jobs.joinAll() // Evil is here, we can't use the first executePendingJob to fail fast
    ...
}

[TelephoneTan]

OK, I got it, without executePendingJob, the unhandled promise rejection handler won't be triggered, so we can't wait until all async jobs get completed to call executePendingJob, we must call executePendingJob immediately after each async job finishes to cancel other jobs fast.

BUT now new question comes: If we call executePendingJob after every single async job, why not remove the executePendingJob in the awaitAsyncJobs? Because executePendingJob is used to handle async job callbacks, and now every async job already has its own executePendingJob.

[TelephoneTan]

OK, I got it, without executePendingJob, the unhandled promise rejection handler won't be triggered, so we can't wait until all async jobs get completed to call executePendingJob, we must call executePendingJob immediately after each async job finishes to cancel other jobs fast.

BUT now new question comes: If we call executePendingJob after every single async job, why not remove the executePendingJob in the awaitAsyncJobs? Because executePendingJob is used to handle async job callbacks, and now every async job already has its own executePendingJob.

I think we can remove the executePendingJob in awaitAsyncJobs to make the code more clear and less confusing:

private suspend fun awaitAsyncJobs() {
    /**
     * This is our simple 'event loop'.
     */
    while (true) {
        val jobs = jobsMutex.withLock { asyncJobs.filter { it.isActive } }
        if (jobs.isEmpty()) {
           // No jobs are running, no jobs will be added
            break
        }
        jobs.joinAll()
        // old jobs finish, new jobs may be introduced by the old jobs
    }
}

the initial jobs are introduced by evaluate, every batch of jobs may introduce new jobs when executing, so we loop until there are no new jobs.

[dokar3]

Yes, this might look good but executePendingJob also executes pure js promise, if an async function or a promise does not call our binding functions, it won't get executed without executePendingJob.

async function call() {}
await call();
console.log("Done");

await new Promise((resolve) => {
resolve();
});
console.log("Done");

We can probably move it out of the while(true) loop if it breaks nothing.

[TelephoneTan]

Then if the user await the Promise using pure JS, the evaluate would never know and return earlier before the Promise gets settled.

[TelephoneTan]

Then if the user await the Promise using pure JS, the evaluate would never know and return earlier before the Promise gets settled.

Even if we use executePendingJob in awaitAsyncJobs, executePendingJob could return 0(currently no jobs), if the user is using pure JS Promise, we can't tell from that if there is any Promise pending, the evaluate may return earlier.

So executePendingJob in evaluate doesn't seem to help if the user uses pure JS Promise.

[dokar3]

It's necessary and it will help. See quickjs-libc's loop: quickjs-libc.c

I will also push some tests later.

[TelephoneTan]

If user doesn't use kotlin asyncFunction, there won't be jobs in asyncJobs, then how could evaluate wait for the js Promise?

[dokar3]

Well, we don't need to wait for them, executePendingJob will execute them without waiting for anything.

[TelephoneTan]

private suspend fun awaitAsyncJobs() {
         jsMutex.withLock {
             do {
                 // Execute JS Promises, putting this in while(true) is unnecessary
                 // since we have the same loop after every asyncFunction call
                 val execResult = executePendingJob(runtime)
                 if (execResult is ExecuteJobResult.Failure) {
                     throw execResult.error
                 }
             } while (execResult == ExecuteJobResult.Success)
         }
         while (true) {
             val jobs = jobsMutex.withLock { asyncJobs.filter { it.isActive } }
             if (jobs.isEmpty()) {
                 // No jobs to run
                break
            }
            jobs.joinAll()
        }
    }

consider JS code like this:

new Promise((rs,re)=>{setTimeout(()=>rs(),10_000)}).then((v)=>console.log("timeout reached"))

There is no kotlin side asyncFunction here, so asyncJobs will always be empty.

What will executePendingJob do when JS code is waiting for timeout? Would it hang? Would it return ExecuteJobResult.Failure or ExecuteJobResult.NoJobs?

If executePendingJob returns ExecuteJobResult.Failure or ExecuteJobResult.NoJobs in this case, awaitAsyncJobs will fall through, return from the break statement, then the evaluate will return earlier than the JS code.

[dokar3]

setTimeout(()=>rs(),10_000)

I don't think we can implement setTimeout without an asyncFunction binding.

[TelephoneTan]

OK, I got it.

The executePendingJob loop in awaitAsyncJobs is used to exhaust the non external-async JS codes and the the callbacks of external-async calls are left to the executePendingJob loop in invokeAsyncFunction.

[dokar3]

Yes, that's right. Once we enter the loop in invokeAsyncFunction, it will execute any jobs.

[dokar3]

Converting to discussion.