quickjs-emscripten icon indicating copy to clipboard operation
quickjs-emscripten copied to clipboard

Feature Request: Interrupt handler (or equivalent) for contexts (not just runtimes)

Open localyost3000 opened this issue 2 years ago • 5 comments
trafficstars

I am working on a feature in our product to allow tracing "time in vm" by code execution, along with the ability to "cancel" running functions if they surpass some limit or if cancellation is manually requested. We currently use a single context and runtime with a singular setInterruptHandler to stop long-running executions, but that doesn't provide enough granularity for our usecase.

I began the work to split our vm into multiple contexts under a singular runtime to benefit from the configured limits, only to discover the interrupt handler lives at the runtime level. Is there any chance that this method or something similar could be ported down to either the evalCode/evalCodeAsync level, or at least the context?

localyost3000 avatar Dec 08 '22 19:12 localyost3000

Commenting on my own Issue to add a possible bug and alter my feature request a bit accordingly.

Essentially, after making this ticket I went ahead and put in the work to make our system use brand new Runtimes for each execution group, but noticed something wasn't working with the interrupt handlers. For some reason, the interrupt handlers set by context.runtime.setInterruptHandler were never firing. Setting one via the option in newRuntime fires the interrupt once, as soon as the evalCodeAsync is called, but never again.

I can set up timers to periodically call back to JS-land, but unfortunately it appears as though the InterruptHandler is the only way to forcibly interrupt execution. Would it be possible to get an interrupt method to forcibly interrupt, or possibly a fix to this interruptHandler issue?

localyost3000 avatar Dec 09 '22 07:12 localyost3000

Maybe the interrupt handler needs to be reinstalled after firing once? I don’t have free time right now to debug this issue.

justjake avatar Dec 13 '22 01:12 justjake

There are tests in the repo that check the basics of vm.runtime.setInterruptHandler here:

https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/ts/quickjs.test.ts#L448

Can you open a PR with a failing test that reproduces the issue you're experiencing? That would go a long way towards fixing the issue.

Is there any chance that this method or something similar could be ported down to either the evalCode/evalCodeAsync level, or at least the context?

It's possible to change QuickJS to pass through the JSContext pointer to the runtime. Here's one way to do it:

  1. You'd need to modify the type of JSInterruptHandler here to take a JSContext *ctx argument: https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/quickjs/quickjs.h#L843
  2. At the interrupt_handler call site here, pass the ctx argument: https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/quickjs/quickjs.c#L6778-L6779
  3. Fix up existing functions that implement JSInterruptHandler to accept the ctx argument. There are a few in the QuickJS source, and one in quickjs-emscripten's C code.
  4. In the Typescript code, add the ctx argument to this typedef: https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/ts/emscripten-types.ts#L148-L151
  5. In the Typescript code, accept the ctx argument here and map it to a JSContext object using this.contextMap.get(ctx), and pass the context object through to the user-defined interruptHandler: https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/ts/runtime.ts#L343-L354
  6. Finally, change the public interface for type InterruptHandler to have a QuickJSContext argument: https://github.com/justjake/quickjs-emscripten/blob/04498f98e743a1ccd420b58c66acb3b580e072b9/ts/runtime.ts#L26-L33

After that, the interruptHandler callback you write can consider the context that's being interrupted, so you can implement context-specific behavior.

justjake avatar Dec 13 '22 16:12 justjake

Thanks @justjake! I appreciate you taking a look at this. I am working on a minimal repro as we speak. I noticed in the unit tests that the interrupt is only checked to have been called once, which does fit what I've seen in our implementation. The interrupt handler is called once, as soon as the runtime is created, but never again. I suspect the issue is related to our use of promises (perhaps the interrupt handler isn't called while the VM is waiting on a promise to resolve?) but am hoping to determine that with the repo case.

Those changes seem doable, so I'll take a stab at a PR in a bit.

localyost3000 avatar Dec 13 '22 17:12 localyost3000

I created a "simple" repo (#90) and it shows what I have been seeing. 0 calls to the interrupt handler when running async code via the async context. Please take a look, and let me know if I am just doing something wrong here. We've got a pretty complex setup in our project and everything works as expected except the interrupts

localyost3000 avatar Dec 13 '22 18:12 localyost3000