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

Evaluation timeout

Open theduke opened this issue 4 years ago • 5 comments

JS_SetInterruptHandler() configures an interrupt handler that can be used to implement a evaluation timeout.

Expose this via a builder api.

theduke avatar Jul 29 '19 20:07 theduke

Hello @theduke, I would be very interested in this feature.

If I understand correctly, the JS_SetInterruptHandler function takes the runtime, a JSInterruptHandler function that must return a != 0 value in case the execution must be interrupted and a opaque pointer to "some" data.

I was looking at implementing this myself, but I stuck on the best way to deal with the unsafe memory management as I understand that the data to which the opaque pointer points, needs to be cleaned up manually afterwards. I'm not very familiar with this yet.

This was my idea to add the timeout feature:

  • Add an option to the builder to configure the timeout (duration)
  • Calculate the time after which the execution must be terminated (current time + timeout duration)
  • Implement a JSInterruptHandler function which would return 1 when current time > max. execution time (stored in opaque pointer)

brocaar avatar Nov 17 '21 21:11 brocaar

Hello @theduke, I would be looking forward to your feedback on my previous message :)

brocaar avatar Dec 08 '21 14:12 brocaar

Hi @brocaar you don't have to bother with the opaque ptr persé, you could just pass a null ptr see https://github.com/HiRoFa/quickjs_es_runtime/blob/master/src/quickjs_utils/interrupthandler.rs#L12

And then deside for yourself if you want to interrupt

Kind regards

andrieshiemstra avatar Dec 08 '21 14:12 andrieshiemstra

Thanks @andrieshiemstra. I'm still a bit lost... I think I need to pass some state like timestamp to the interrupt handler such that the interrupt handler when called, can decide the execution must be terminated or not (e.g. SystemTime::now() > max_runtime). My thinking was that the opaque pointer would be suitable for this use-case, e.g. to store the max_runtime. The other way would be to pass a closure function which captures this state, but I don't think it is possible to pass a closure function directly as it has to be a "C" fn.

I see that in your case, the interrupt handler is executed through QuickJsRuntimeAdapter::do_with. My feeling is that if I don't want to bother with the opaque ptr, I need to implement something like QuickJsRuntimeAdapter::do_with to be able to use a closure function but it looks like there is quite some logic behind that too.

Maybe I'm looking in the wrong direction and if so, would you be able to provide a code example how this could be implemented?

brocaar avatar Dec 13 '21 21:12 brocaar

Actually, maybe it is as easy as this?

extern "C" fn timeout_interrupt(
    rt: *mut q::JSRuntime,
    opaque: *mut c_void,
) -> ::std::os::raw::c_int {
    let ts: SystemTime = unsafe { std::ptr::read(opaque as *const SystemTime) };
    if SystemTime::now().gt(&ts) {
        1
    } else {
        0
    }
}

And to set the interrupt:

            let timeout = Box::new(SystemTime::now() + timeout);
            let ptr = Box::into_raw(timeout);

            unsafe {
                q::JS_SetInterruptHandler(runtime, Some(timeout_interrupt), ptr as *mut c_void);
            }

My initial issue was that I was using Box::from_raw, but that can only be called once as the data is dropped when it goes out of scope. It looks like std::ptr::read is the solution for this.

If I'm not mistaken, to cleanup the timeout value, I can run the following after the JS execution has been completed:

let _ = unsafe { Box::from_raw(ptr) };

brocaar avatar Dec 13 '21 22:12 brocaar