RFC: TaskBuilder

[kjvalencik]

The Task API was introduced in Neon v0.4.0 and has proven to be incredibly valuable to developers. It empowers users to execute long running tasks on the libuv thread pool without blocking the main javascript thread.

The TaskBuilder API expands upon this concept with an improved ownership model and ergonomics.

pub fn add_one(mut cx: FunctionContext) -> JsResult<JsUndefined> {
    let n = cx.argument::<JsNumber>(0)?.value();
    let cb = cx.argument::<JsFunction>(1)?;

    cx.task(move || {
            let result = n + 1.0;

            move |mut cx| Ok(cx.number(result))
        })
        .schedule(cb)
}

Rendered

Proof of concept implementation

[qm3ster]

For me, this is more straightforward in terms of "what must I implement?" for a new user, yet terser.
As long as there aren't difficulties with lifetimes (and there shouldn't be) this seems like the way forward.
I do see it as a general quality of life improvement and not directly a solution to neon-bindings/neon#283.

[kjvalencik]

@qm3ster Thanks for taking a look!

I don't think there will be issues with lifetimes in the API itself, but, I do think there's potential to make issues with lifetimes in the user's code more difficult to read.

Error messages on closures can be pretty gnarly, especially when inference failed. For example, I could see a new user failing to move the environment on the closure. That type of mistake is more obvious on the trait because it's explicit.

It may be even less obvious because trivial examples (like mine above) will only use types that are Copy. This means it will work until the user tries to do something more complicated.

[qm3ster]

I was accidentally considering someone new to Neon but experienced with Rust closures.
But I definitely see how people just learning Rust to extend their node.js are very likely to be using Neon and how that would be a problem.
My counter is that they would neither be able to call cx.task nor implement Task by hand without prescriptive documentation at that point, so as long as move is accented upon it should be fine, even without going into detail on how closures live in memory.

[kjvalencik]

That's an excellent point. New users are likely to copy paste an example. As long as all of our examples include move (even where unnecessary--e.g., types implement copy or references are 'static), we should be in good shape.

If you use move, but the field isn't Send the error is fairly clear. And to your point, they would encounter the exact same issue with the Task trait.

Thanks for the thoughtful response!

[dherman]

This RFC looks awesome. I think it's going to be a massive improvement to the Task API ergonomics, and a big selling point for Neon. Imagine how impressive it will be to show off how trivially you can execute lightning-fast Rust computations on a background thread!

There were a few details I didn't fully understand, so I made a few suggestions for clarifications.

[dherman]

Mention that it's still more expressive, e.g.: "This simplifies infallible tasks and is still expressive enough to allow users to use a Result type to represent fallible tasks."

[dherman]

Let's mention why (it's less convenient and less powerful) and also that we won't remove it until Neon users have had enough time to migrate to the new API.

[dherman]

Can you include the full TaskBuilder type declaration and the signatures of the methods in the RFC?

[dherman]

I don't think I understand this. The schedule method returns Handle<JsUndefined>? Or the JS callback is wrapped to discard its result and return undefined? Or something else? It might be helpful to include the type signature.

[kjvalencik]

I'll write more examples. schedule returns undefined to remove a little more boilerplate of creating that value since that's the most common pattern.

[kjvalencik]

pub fn perform_closure_task(mut cx: FunctionContext) -> JsResult<JsValue> {
    let n = cx.argument::<JsNumber>(0)?.value();
    let cb = cx.argument::<JsFunction>(1)?;

    cx.task(move || {
            let result = n + 1.0;

            move |mut cx| Ok(cx.number(result))
        })
        .schedule(cb)
}

[kjvalencik]

I need to update the RFC, but, I made it return a JsUndefined upcast to JsValue. This makes it fit a few more situations. For example, class methods always return JsValue.

[dherman]

I think this section would be easier to understand if we had the real type declaration that this RFC is proposing documented above, to compare and contrast with.

[kjvalencik]

I'm going to try a little bit more to fix this issue before resigning to the more limited version.

[dherman]

@qm3ster I'm curious about this comment:

not directly a solution to neon-bindings/neon#283.

What is missing from the RFC that doesn't solve those problems? I understood this to be strictly more powerful than an &mut self, since it provides an owned self.

[qm3ster]

@dherman Ah, I just meant that it is more flexible than that, but also would still be boilerplate and readability improvement even if it gave only an immutable reference.

[dherman]

We discussed this with the core team yesterday: I think it's important for the name of this method to indicate that it's being scheduled in Node's core thread pool, so it's clear this isn't just an arbitrary thread. In particular this helps people understand that it's competing with Node's I/O threads for processing time.

@goto-bus-stop proposed the name .node_task(), which I like. I don't think we want to expose libuv in the name, because that's too low-level. And it's not necessarily critical to be explicit about the pool itself, which is also a bit of a low-level detail. But the point is this is a task that's part of Node's internal task scheduler. Also the word "task" does seem to be consistent with the language used by the relevant N-API APIs' documentation (the APIs themselves use the term "work" which is a little awkward here, but the prose underneath uses the word "task"), so it seems reasonably idiomatic for the Node platform.

[dherman]

A thought from a call with @kjvalencik today: can we refactor this API to look a little closer to the threadsafe handles API, for a more consistent overall design?

[kjvalencik]

Superseded by #35