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 https://github.com/neon-bindings/neon/pull/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]

@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]

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 https://github.com/neon-bindings/rfcs/pull/35