TaskBuilder and JsPromise

[kjvalencik]

API for executing tasks on the libuv threadpool and settling JavaScript Promise.

fn return_a_promise(cx: FunctionContext) -> JsResult<JsPromise> {
    let promise = cx
        .task(|| { /* ... */)
        .promise(|mut cx, _result| Ok(cx.undefined()));
    
    Ok(promise)
}

Implementation PR: neon-bindings/neon#789

[dherman]

This is freaking amazing.

[kjvalencik]

For high level APIs that reduce complexity and closure nesting, I propose:

// Wraps with a `try_catch` and resolves/rejects based on the `JsResult`
// Without GAT, because of the lifetime on the result, this might need to return `JsValue` instead of `T: Value`
Deferred::and_settle(self, f: impl FnOnce(cx: TaskContext) -> JsResult);

// On queue, to remove the `.send(move || {})` closure boilerplate:
// Calls `Deferred::and_settle` for you
EventQueue::send(&self, f: Impl FnOnce....)

[dherman]

A couple small suggestions, but it's very close!

[dherman]

🎉

[kjvalencik]

A couple of potential issues with this RFC.

Lifetime Inference

Since the closure in settle_with takes a borrowed TaskContext, users may run into a lifetime inference issue if the closure is saved to a variable instead of passed directly to the function.

395 |                 .try_settle_with(deferred, callback)
    |                  ^^^^^^^^^^^^^^^ one type is more general than the other
    |
    = note: expected enum `Result<neon::handle::Handle<'_, _>, _>`
               found enum `Result<neon::handle::Handle<'a, _>, _>`

The workaround is pretty ugly:

fn constrain_callback<F>(f: F) -> F
where
    for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, JsString>,
{
    f
}

This issue is avoided if an owned TaskContext is passed. Since Neon owns the lifetime, we can give an owned TaskContext to ease this case. We may also want to consider doing the same in try_catch.

This is a breaking change and needs to be decided prior to merging.

Promises and synchronous exceptions

One advantage of using async function() {} over a function() {} that returns a promise is that the function is guaranteed to always return a Promise. Synchronous exceptions are transparently converted into rejections. This is cumbersome to do in Neon because it requires using try_catch and handling the result. I propose a new helper JsPromise::try_catch for performing this.

fn never_throws_synchronously(mut cx: FunctionContext) {
    Ok(JsPromise::try_catch(|cx| {
        let n = cx.argument::<JsNumber>(0)?.value(cx);
        
        cx.task(move || n + 42)
            .promise(|cx, n| Ok(cx.number(n)))
    }))
}

The downside of this approach is that it still requires careful code to avoid an unhandledRejection when using cx.promise(). The (deferred, promise) pair shouldn't be created until after all code that can be thrown synchronously.

Alternatively, we could provide a Deferred::try_catch that uses the existing promise to avoid this gotcha. However, I'm not sure how to align these two use cases. A user could still stumble onto the JsPromise::try_catch version which is better suited towards TaskBuilder::promise.

fn never_throws_synchronously(mut cx: FunctionContext) {
    Ok(cx.try_promise(|cx, deferred| {
        let n = cx.argument::<JsNumber>(0)?.value(cx);
        let channel = cx.channel();
        
        std::thread::spawn(move || {
            let n = n + 42;
            
            channel.settle_with(deferred, move |cx| Ok(cx.number(n)));
        });

        Ok(())
    }))
}

This is an extension and can be added later without causing any breaking changes.

[dherman]

@kjvalencik So you mean settle_with and try_settle_with should take an owned TaskContext, yes? That seems fine to me.

We may also want to consider doing the same in try_catch.

Are you talking about a backwards-incompatible change to the existing cx.try_catch() API, or are you talking about one of the try_catch proposals you're making in the second half of your comment? I'm not quite following the "Promises and synchronous exceptions" section, it seems like there are several different hypothetical APIs in play here, some of which might be typos but I'm not sure?

• JsPromise::try_catch
• Deferred::try_catch
• Context::try_promise

This sentence was hard to follow in particular:

A user could still stumble onto the JsPromise::try_catch version which is better suited towards TaskBuilder::promise.

[kjvalencik]

@dherman I meant a breaking change to cx.try_catch (which is still feature flagged).

These are several different proposals for similar things. The idea is we should make it easy for a user to write a Neon function that always resolved or rejects a promise instead of throwing an error synchronously.

The issue I'm having is I haven't been able to come up with a single API that works for both TaskBuilder and Context::deferred.

[kjvalencik]

After some testing, moving to an owned TaskContext does not resolve this issue. With that said, I think it should still get an owned TaskContext for consistency.

[kjvalencik]

Merged in 2e6c1f1