Implement Futures RFC

[kjvalencik]

Implements #872

Limitation

This makes use of JsFunction::new with a closure, which requires Node-API 5. This seemed like a reasonable limitation to simplify the implementation.

However, it would also be possible by passing the state with a JsBox and using Function.prototype.bind.

[dherman]

This makes use of JsFunction::new with a closure, which requires Node-API 5. This seemed like a reasonable limitation to simplify the implementation.

This should be fine IMO. Node 14 has had support for Node-API 6 since 14.0.0, and Node 12 is EOL at the end of next month. So it's not worth the extra complexity to support older versions of Node that are about to be EOL.

[kjvalencik]

@dherman This has been updated:

• Resolving conflicts from latest main
• Switching to tokio::sync::oneshot and allowing JoinHandle::join and .await to live side-by-side.

This introduces a side-effect that calling .join() from an asynchronous context will panic. This is nearly always a programmer error. It could theoretically be in a dependent crate that they can't control, but the user has few options to resolve:

• Disable the futures feature (which might not be an option if they are using it)
• PR the upstream dependency to at a futures support that toggles appropriately
• Use rt.spawn_blocking(..) to push it to a blocking thread and outside the runtime

[dherman]

I really like where this API has landed. I left a few suggestions for clarity, because some of the code we a little hard for me to follow.

[dherman]

Minor semantic proposal: since getting a property could trigger arbitrary JS side effects (since, for example, it could be a getter function), I think we should perform the side effects in this order:

1. Extract the .then property
2. Exec the then function
3. Extract the .catch property
4. Exec the catch function

[kjvalencik]

I went with this order to avoid calling anything if either getter fails. This is to avoid being in a half successful state.

It's still possible to be in this state if calling .then fails, but at least it's less likely.

[dherman]

OK, I see where you're coming from. I don't think I'm quite convinced but I also think it's not worth blocking progress.

[dherman]

This now means we only test one path through the codebase, when the runtime is implemented differently depending on whether this flag is enabled or disabled. We should probably run our tests both with and without the "futures" flag. IMO it's fine if we do this in follow-up work, since our next release will be a beta so the risk is somewhat mitigated.

[dherman]

🚢

[dherman]

OK, I see where you're coming from. I don't think I'm quite convinced but I also think it's not worth blocking progress.

[dherman]

Nice!

[kjvalencik]

I will follow-up with a PR to tests since it will require some refactoring to use multiple Neon modules in the test suite.

In the meantime, I tested with a small example locally:

use neon::prelude::*;

fn lazy_add(mut cx: FunctionContext) -> JsResult<JsUndefined> {
    let a = cx.argument::<JsFunction>(0)?.root(&mut cx);
    let b = cx.argument::<JsFunction>(1)?.root(&mut cx);
    let cb = cx.argument::<JsFunction>(2)?.root(&mut cx);
    let channel = cx.channel();

    std::thread::spawn(move || {
        let (a, b, cb) = channel
            .send(move |mut cx| {
                let a = a.into_inner(&mut cx);
                let b = b.into_inner(&mut cx);
                let cb = cb.into_inner(&mut cx);

                let a = a.call_with(&cx).apply::<JsNumber, _>(&mut cx)?;
                let b = b.call_with(&cx).apply::<JsNumber, _>(&mut cx)?;

                Ok((a.value(&mut cx), b.value(&mut cx), cb.root(&mut cx)))
            })
            .join()
            .unwrap();

        let c = a + b;

        channel.send(move |mut cx| {
            cb.into_inner(&mut cx)
                .call_with(&cx)
                .arg(cx.number(c))
                .exec(&mut cx)?;

            Ok(())
        });
    });

    Ok(cx.undefined())
}

#[neon::main]
fn main(mut cx: ModuleContext) -> NeonResult<()> {
    cx.export_function("lazyAdd", lazy_add)?;

    Ok(())
}

const addon = require(".");

addon.lazyAdd(() => 1, () => 2, console.log);