Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(neon): Add tokio async runtime support #1055

Open
wants to merge 2 commits into
base: kv/clippy
Choose a base branch
from

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Jul 5, 2024

This PR adds support for exporting async fn and future return fn in #[neon::export].

#[neon::export]
async fn example1(a: f64, b: f64) -> f64 {
    a + b
}

#[neon::export(async)]
fn example2(a: f64, b: f64) -> impl Future<Output = f64> {
    async move {
        a + b
    }
}

It is intentionally very conservative in semver guarantees. It requires registering a global future executor. The executor must implement the private Runtime trait. Currently, only a tokio implementation is included. Additionally, Future trait bounds are strict enough to allow for multi-threaded executors.

As we see demand, we can add support for additional executors (e.g., async-std) and possibly even single threaded executors (#[neon::export(?Sync)]) or a built-in libuv driven executor. The eventual goal is to make the Runtime trait public so that users can bring their own executor.

@kjvalencik kjvalencik changed the title feat(neon): At tokio async runtime support feat(neon): Add tokio async runtime support Jul 8, 2024
@kjvalencik kjvalencik force-pushed the kv/async-runtime branch 2 times, most recently from 3da8a9e to 3cfbd7a Compare July 16, 2024 19:43
@kjvalencik kjvalencik force-pushed the kv/async-runtime branch 2 times, most recently from c17a9ad to 394b695 Compare September 10, 2024 16:37
@kjvalencik kjvalencik marked this pull request as ready for review September 13, 2024 16:33
/// # use std::future::Future;
/// # use neon::prelude::*;
/// #[neon::export(async)]
/// fn add(_cx: &mut FunctionContext, a: f64, b: f64) -> NeonResult<impl Future<Output = f64>> {
Copy link
Member Author

@kjvalencik kjvalencik Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dherman This doesn't need the unused _cx, but I was trying to show that it can use a cx. A few options:

  • Leave it as is
  • Switch to console.log to demonstrate using it
  • Remove it
  • Comment it out to show that it could be used but isn't required
Suggested change
/// fn add(_cx: &mut FunctionContext, a: f64, b: f64) -> NeonResult<impl Future<Output = f64>> {
/// fn add(/* cx: &mut FunctionContext, */ a: f64, b: f64) -> NeonResult<impl Future<Output = f64>> {

What do you think?

Similarly, NeonResult isn't used here. Originally it was because I planned on calling console.log, but then I left it in. I think maybe it should be removed? Let me know, I was trying not to have 100 different examples.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I feel like we should have a shorthand for the console, so you could do:

cx.console()?.log(&mut cx, "Hello from the main JavaScript thread!")?;

But for now why don't we just remove the cx argument entirely and the NeonResult, and we can add it back after we merge #1056, because then we could at least write:

    cx.global("console")?
        .get(&mut cx, "log")?
        .bind(&mut cx)
        .arg("Hello from the main JavaScript thread!")
        .apply()?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think it's probably best to leave them out for good. The docs don't talk about taking a FunctionContext until the bottom of the page, so it's best not to jump ahead. IMO we should keep it simple with println! for these examples.

@kjvalencik kjvalencik force-pushed the kv/async-runtime branch 2 times, most recently from e1f61ca to 1cc69a4 Compare September 16, 2024 21:33
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small changes, but nothing major. TBH I'm a little worried my lack of expertise at tokio and async means my review might be a little superficial. But the API is so easy to understand and fits so nicely with the #[export] macro syntax. This is a really exciting addition.

crates/neon/src/context/mod.rs Outdated Show resolved Hide resolved
crates/neon/src/lib.rs Show resolved Hide resolved
crates/neon/src/lib.rs Show resolved Hide resolved
crates/neon/src/lifecycle.rs Outdated Show resolved Hide resolved
crates/neon/src/macro_internal/futures.rs Outdated Show resolved Hide resolved
crates/neon/src/sys/bindings/mod.rs Outdated Show resolved Hide resolved
crates/neon/src/types_impl/extract/private.rs Outdated Show resolved Hide resolved
test/napi/src/lib.rs Outdated Show resolved Hide resolved
/// # use std::future::Future;
/// # use neon::prelude::*;
/// #[neon::export(async)]
/// fn add(_cx: &mut FunctionContext, a: f64, b: f64) -> NeonResult<impl Future<Output = f64>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think it's probably best to leave them out for good. The docs don't talk about taking a FunctionContext until the bottom of the page, so it's best not to jump ahead. IMO we should keep it simple with println! for these examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants