From d94405ceed05d6749e6fc96fcbe38a95573bce20 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Thu, 17 Sep 2020 17:38:40 -0400 Subject: [PATCH 1/8] JsPromise --- text/0000-promise.md | 260 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 260 insertions(+) create mode 100644 text/0000-promise.md diff --git a/text/0000-promise.md b/text/0000-promise.md new file mode 100644 index 0000000..416cedd --- /dev/null +++ b/text/0000-promise.md @@ -0,0 +1,260 @@ +- Feature Name: `JsPromise` +- Start Date: 2020-09-17 +- RFC PR: (leave this empty) +- Neon Issue: (leave this empty) + +# Summary +[summary]: #summary + +Provide an API for creating, resolving and rejecting JavaScript Promises. + +```rust +fn return_a_promise(cx: FunctionContext) -> JsResult { + let (promise, deferred) = cx.promise(); + let msg = cx.string("Hello, World!"); + + deferred.resolve(&cx, msg); + + Ok(promise) +} +``` + +# Motivation +[motivation]: #motivation + +JavaScript Promise support has been a [long requested feature](https://github.com/neon-bindings/neon/issues/73). Promises are desirable for several reasons: + +* Considered best practices in idiomatic JavaScript +* Enable `await` syntax for asynchronous operations +* More easily map to asynchronous operations in native code + +Additionally, they can be combined with the [`EventQueue`](https://github.com/neon-bindings/rfcs/pull/32) API for very simple asynchronous threaded usage. + +```rust +fn real_threads(cx: FunctionContext) -> JsResult { + let queue = cx.queue(); + let (promise, deferred) = cx.promise(); + + std::thread::spawn(move || { + let result = perform_complex_operation(); + + queue.send(move || { + let result = cx.number(result); + + deferred.resolve(&cx, result); + }); + }); + + Ok(promise) +} +``` + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Before `Promise`, callbacks of the form `function (err, value)` were very common in JavaScript. Neon has excellent for for these "node style" callbacks in the `Task` trait. + +```rust +fn fibonacci_async(mut cx: FunctionContext) -> JsResult { + let n = cx.argument::(0)?.value() as usize; + let cb = cx.argument::(1)?; + + FibonacciTask::new(n).schedule(cb); + + Ok(cx.undefined()) +} +``` + +However, in idiomatic JavaScript, this should method should return a promise. This can be solved with some glue code in JavaScript: + +```js +const util = require('util'); +const native = require('../native'); + +export const fibonacciAsync = util.promisify(native.fibonacci_async); +``` + +Alternatively, a `Promise` can be constructed directly in Neon. Unlike an asynchronous method that accepts a callback, a method that returns a promise requires two parts: + +* A `Promise` to return from the method +* A hook to resolve or reject the `Promise` + +In JavaScript, this looks like the following: + +```js +function asyncMethod() { + return new Promise((resolve, reject) => { + if (Math.random() > 0.5) { + resolve(); + } else { + reject(); + } + }); +} +``` + +In `Neon`, when a `JsPromise` is constructed, a `Deferred` object is also provided. It can be thought of as the following pattern in JavaScript: + +```js +function deferred() { + const deferred = {}; + const promise = new Promise((resolve, reject) => { + deferred.resolve = resolve; + deferred.reject = reject; + }); + + return [promise, deferred]; +} + +function asyncMethod() { + const [promise, d] = deferred(); + + setTimeout(() => d.resolve(), 5000); + + return promise; +} +``` + +This could be written in Neon with the following: + +```rust +fn async_method(cx: FunctionContext) -> JsResult { + let queue = cx.queue(); + let (promise, d) = cx.promise(); + + std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(5000)); + + queue.send(|cx| d.resolve(cx.undefined())); + }); + + Ok(promise) +} +``` + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +The `JsPromise` API consists of two structs: + +* `JsPromise`. Opaque value; only useful for passing back to JavaScript. +* `Deferred`. Handle for resolving and rejecting the related `Promise`. + +They may only be created with the `deferred` method on `Context`. + +```rust +trait Context { + fn deferred(&mut self) -> (Handle, Deferred); +} +``` + +## `JsPromise` + +Opaque handle that represents a JavaScript `Promise`. It is not threadsafe. + +```rust +/// A JavaScript `Promise` +#[repr(C)] +#[derive(Clone, Copy)] +pub struct JsPromise(raw::Local); + +impl Value for JsPromise {} + +impl Managed for JsPromise { + fn to_raw(self) -> raw::Local { self.0 } + fn from_raw(h: raw::Local) -> Self { JsPromise(h) } +} + +impl ValueInternal for JsPromise { + fn name() -> String { "promise".to_string() } + fn is_typeof(env: Env, other: Other) -> bool { + unsafe { neon_runtime::tag::is_promise(env.to_raw(), other.to_raw()) } + } +} + +unsafe impl This for JsPromise { + fn as_this(_env: Env, h: raw::Local) -> Self { + JsPromise(h) + } +} + +impl JsPromise { + pub(crate) fn new_internal<'a>(value: raw::Local) -> Handle<'a, JsPromise> { + Handle::new_internal(JsPromise(value)) + } +} +``` + +## `Deferred` + +`Send` handle for resolving or rejecting a `JsPromise`. + +```rust +pub struct Deferred(*mut c_void); + +unsafe impl Send for Deferred {} + +impl Deferred { + /// Resolves a deferred Promise with the value + fn resolve<'a, C: Context<'a>, T: Value>(self, cx: &mut C, v: Handle); + + /// Rejects a deferred Promise with the error + fn reject<'a, C: Context<'a>, E: Value>(self, cx: &mut C, err: Handle); + + /// Resolves or rejects a deferred Promise with the contents of a `Result` + fn complete<'a, C: Context<'a>, T: Value, E: Value>(self, cx: &mut C, result: Result); +} +``` + +Similar to [`Persistent`](https://github.com/neon-bindings/rfcs/pull/32), if a `Deferred` is not resolved or rejected, the `Promise` will leak. To help catch this and guide users towards a correct implementation, `Deferred` should `panic` on `Drop` if not used. + +```rust +impl std::ops::Drop for Deferred { + fn drop(&mut self) { + panic!("JsPromise leaked. Deferred must be used."); + } +} +``` + +# Drawbacks +[drawbacks]: #drawbacks + +None? :grin: + +# Rationale and alternatives +[alternatives]: #alternatives + +## High Level Promise Tasks + +Using `JsPromise` along with other async features requires careful and verbose usage of several features (`JsPromise`, `EventQueue` and `try_catch`). Neon could exclusively provide a high-level API similar to `Task` or the proposed `TaskBuilder`. + +```rust +fn async_task(cx: FunctionContext) -> JsResult { + let promise = cx.task(|| /* perform async task */) + .complete(|cx| /* convert to js types */); + + Ok(promise) +} +``` + +This API is very ergonomic, but removes a considerable amount of flexibility and power from the user. Instead, the initial implementation will focus on orthogonal core primitives like `Persistent`, `EventQueue`, `JsBox` and `JsPromise` that can later be combined into high-level APIs. + +## Common `Deferred` pattern + +Most promise libraries that provide a `Deferred` object provide the `promise` as part of that object. Neon might have a similar approach: + +```rust +struct Deferred<'a> { + handle: *mut c_void, + promise: Handle<'a, JsPromise>, +} +``` + +The issue with this approach is that `Deferred` is `Send` and `JsPromise` is *not*. We would also need to provide a getter for the resolve/reject handle. That getter would need to _consume_ self because it cannot be used multiple times. This would result in worse ergonomics. + +Instead a tuple is returned, similar to `(sender, receiver)` returned from `std::sync::mpsc::channel`. + +# Unresolved questions +[unresolved]: #unresolved-questions + +- Should there be a constructor method on `JsPromise`? This is slightly awkward because it's impossible to create a `Promise` without also creating a `Deferred`. From 7e9033eb5ed7ecb0680ac0f2fae987d87c14184e Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Thu, 9 Sep 2021 17:15:00 -0400 Subject: [PATCH 2/8] Add TaskBuilder and general updates --- text/0000-promise.md | 292 +++++++++++++++++++++++++++++++++---------- 1 file changed, 224 insertions(+), 68 deletions(-) diff --git a/text/0000-promise.md b/text/0000-promise.md index 416cedd..da183d2 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -1,77 +1,102 @@ -- Feature Name: `JsPromise` +- Feature Name: `JsPromise` and `TaskBuilder` - Start Date: 2020-09-17 - RFC PR: (leave this empty) - Neon Issue: (leave this empty) -# Summary +## Summary [summary]: #summary +Provides an ergonomic API for executing tasks on the libuv threadpool and notifying JavaScript on the main thread with a result. + +This RFC provides for two related, but distinct features: `JsPromise` and `TaskBuilder`. Since users will frequently be interacting with both, designing the two in tandem will result in a better design. + +### `JsPromise` + Provide an API for creating, resolving and rejecting JavaScript Promises. ```rust fn return_a_promise(cx: FunctionContext) -> JsResult { - let (promise, deferred) = cx.promise(); + let (deferred, promise) = cx.promise(); let msg = cx.string("Hello, World!"); - deferred.resolve(&cx, msg); + deferred.resolve(&mut cx, msg); Ok(promise) } ``` -# Motivation +### `TaskBuilder` + +Provide an API for executing Rust closures on the libuv threadpool. + +```rust +cx.task(|| { /* Executes asynchronously on the threadpool */ }) + .complete(|cx, result| { + // Executes on the main JavaScript thread + Ok(()) + }); +``` + +## Motivation [motivation]: #motivation +### `JsPromise` + JavaScript Promise support has been a [long requested feature](https://github.com/neon-bindings/neon/issues/73). Promises are desirable for several reasons: * Considered best practices in idiomatic JavaScript * Enable `await` syntax for asynchronous operations * More easily map to asynchronous operations in native code -Additionally, they can be combined with the [`EventQueue`](https://github.com/neon-bindings/rfcs/pull/32) API for very simple asynchronous threaded usage. +### `TaskBuilder` -```rust -fn real_threads(cx: FunctionContext) -> JsResult { - let queue = cx.queue(); - let (promise, deferred) = cx.promise(); +The legacy backend for Neon provided the [`Task`](https://docs.rs/neon/0.8.3/neon/task/trait.Task.html) trait for executing tasks on the libuv threadpool. However, boilerplate heavy implementations and ergonomic issues with ownership inspired the [`TaskBuilder` RFC](https://github.com/neon-bindings/rfcs/pull/30). This RFC is a successor, targeting only the Node-API backend with a heavy emphasis on promises. - std::thread::spawn(move || { - let result = perform_complex_operation(); - - queue.send(move || { - let result = cx.number(result); +Originally, it was thought that `Channel` could replace the need for running tasks on the libuv pool; however, there are many benefits to providing this functionality as well: - deferred.resolve(&cx, result); - }); - }); +* Better CPU scheduling with a single task queue +* Easier integration for users that do not have a preferred threadpool - Ok(promise) -} -``` +More details are available in the Node.js [guides](https://nodejs.org/en/docs/guides/dont-block-the-event-loop/#don-t-block-the-worker-pool). -# Guide-level explanation +## Guide-level explanation [guide-level-explanation]: #guide-level-explanation -Before `Promise`, callbacks of the form `function (err, value)` were very common in JavaScript. Neon has excellent for for these "node style" callbacks in the `Task` trait. +### `JsPromise` + +Before `Promise`, callbacks of the form `function (err, value)` were very common in JavaScript. These callbacks are the predominant form of continuation in Neon. ```rust -fn fibonacci_async(mut cx: FunctionContext) -> JsResult { - let n = cx.argument::(0)?.value() as usize; - let cb = cx.argument::(1)?; +fn task(mut cx: FunctionContext) -> JsResult { + let name = cx.argument::(0)?.value(&mut cx); + let cb = cx.argument::(1)?.root(&mut cx); + let channel = cx.channel(); + + std::thread::spawn(move || { + let greeting = format!("Hello, {}!", name); + + channel.send(move |mut cx| { + let cb = cb.into_inner(&mut cx); + let this = cx.undefined(); + let args = [cx.null().upcast::(), cx.string(greeting).upcast()]; + + cb.call(&mut cx, this, args)?; - FibonacciTask::new(n).schedule(cb); + Ok(()) + }); + }); Ok(cx.undefined()) } ``` -However, in idiomatic JavaScript, this should method should return a promise. This can be solved with some glue code in JavaScript: +However, in idiomatic JavaScript, this method should return a promise. This can be solved with some glue code in JavaScript: ```js const util = require('util'); const native = require('../native'); -export const fibonacciAsync = util.promisify(native.fibonacci_async); +export const taskAsync = util.promisify(native.task); ``` Alternatively, a `Promise` can be constructed directly in Neon. Unlike an asynchronous method that accepts a callback, a method that returns a promise requires two parts: @@ -103,11 +128,11 @@ function deferred() { deferred.reject = reject; }); - return [promise, deferred]; + return [deferred, promise]; } function asyncMethod() { - const [promise, d] = deferred(); + const [d, promise] = deferred(); setTimeout(() => d.resolve(), 5000); @@ -119,36 +144,102 @@ This could be written in Neon with the following: ```rust fn async_method(cx: FunctionContext) -> JsResult { - let queue = cx.queue(); - let (promise, d) = cx.promise(); + let channel = cx.channel(); + let (deferred, promise) = cx.promise(); std::thread::spawn(move || { std::thread::sleep(std::time::Duration::from_millis(5000)); - queue.send(|cx| d.resolve(cx.undefined())); + channel.send(move |mut cx| { + let value = cx.undefined(); + deferred.resolve(&mut cx, value); + Ok(()) + }); }); Ok(promise) } ``` -# Reference-level explanation +However, there are still some subtle gotchas. What if there is a JavaScript exception? What if I don't resolve the `Deferred`? To help, `Deferred::settle_with` is provided as a helper that combines `cx.try_catch(...)` with `Deferred` resolution or rejection: + +```rust +channel.send(move |mut cx| { + deferred.settle_with(&mut cx, |cx| Ok(cx.undefined())); +}); +``` + +### `TaskBuilder` + +Tasks are asynchronously executed units of work performed on the libuv threadpool. A task consists of two parts: + +* An `execute` callback that runs on the libuv threadpool +* A `complete` callback that receives the result of `execute` and runs on the main JavaScript thread + +The `execute` code is typically the slow _native_ operation asynchronously executed, while `complete` handles converting back to JavaScript values and resuming JavaScript execution with the result. + +For example, consider the previous `JsPromise` example that executes on a Rust thread. A similar result can be achieved by running on the libuv threadpool instead: + +```rust +fn async_method(cx: FunctionContext) -> JsResult { + let (deferred, promise) = cx.promise(); + + cx.task(|| std::thread::sleep(std::time::Duration::from_millis(5000))) + .complete(move |cx, _result| { + let value = cx.undefined(); + deferred.resolve(&mut cx, value); + Ok(()) + }); + + Ok(promise) +} +``` + +However, since most of the time tasks will complete by settling a promise, a convenience method is provided to handle creating and settling a promise: + +```rust +fn async_method(cx: FunctionContext) -> JsResult { + let promise = cx + .task(|| std::thread::sleep(std::time::Duration::from_millis(5000))) + .promise(|cx, _result| Ok(cx.undefined())); + + Ok(promise) +} +``` + +`TaskBuilder::complete` and `TaskBuilder::promise` are both thunks that consume the builder, creating and queueing the task. + +## Reference-level explanation [reference-level-explanation]: #reference-level-explanation +### `JsPromise` + The `JsPromise` API consists of two structs: * `JsPromise`. Opaque value; only useful for passing back to JavaScript. * `Deferred`. Handle for resolving and rejecting the related `Promise`. -They may only be created with the `deferred` method on `Context`. +They are most idiomatically crated by `cx.promise()`, but construction is mirrored across `JsPromise` and `Deferred`. ```rust trait Context { - fn deferred(&mut self) -> (Handle, Deferred); + fn promise(&mut self) -> (Deferred, Handle); +} + +impl JsPromise { + pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { + todo!() + } +} + +impl Deferred { + pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { + todo!() + } } ``` -## `JsPromise` +#### `JsPromise` Opaque handle that represents a JavaScript `Promise`. It is not threadsafe. @@ -172,23 +263,21 @@ impl ValueInternal for JsPromise { } } -unsafe impl This for JsPromise { - fn as_this(_env: Env, h: raw::Local) -> Self { - JsPromise(h) - } -} +impl Object for JsPromise {} impl JsPromise { - pub(crate) fn new_internal<'a>(value: raw::Local) -> Handle<'a, JsPromise> { - Handle::new_internal(JsPromise(value)) + pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { + todo!() } } ``` -## `Deferred` +#### `Deferred` `Send` handle for resolving or rejecting a `JsPromise`. +Resolving a `Deferred` always takes ownership of `self` and consumes the `Deferred` to prevent double settlement. + ```rust pub struct Deferred(*mut c_void); @@ -196,17 +285,21 @@ unsafe impl Send for Deferred {} impl Deferred { /// Resolves a deferred Promise with the value - fn resolve<'a, C: Context<'a>, T: Value>(self, cx: &mut C, v: Handle); + pub fn resolve<'a, T: Value, C: Context<'a>>(self, cx: &mut C, v: Handle); /// Rejects a deferred Promise with the error - fn reject<'a, C: Context<'a>, E: Value>(self, cx: &mut C, err: Handle); - - /// Resolves or rejects a deferred Promise with the contents of a `Result` - fn complete<'a, C: Context<'a>, T: Value, E: Value>(self, cx: &mut C, result: Result); + pub fn reject<'a, E: Value, C: Context<'a>>(self, cx: &mut C, err: Handle); + + /// Resolves or rejects a Promise with the result of a closure + pub fn settle_with<'a, T, C, F>(self, cx: &mut C, f: F) + where + T: Value, + C: Context<'a>, + F: FnOnce(&mut C) -> JsResult<'a, T>; } ``` -Similar to [`Persistent`](https://github.com/neon-bindings/rfcs/pull/32), if a `Deferred` is not resolved or rejected, the `Promise` will leak. To help catch this and guide users towards a correct implementation, `Deferred` should `panic` on `Drop` if not used. +Similar to [`Root`](https://github.com/neon-bindings/rfcs/pull/32), if a `Deferred` is not resolved or rejected, the `Promise` chain will leak. To help catch this and guide users towards a correct implementation, `Deferred` should `panic` on `Drop` if not used on Node-API < 4 and be dropped on a global drop queue in Node-API >= 4. ```rust impl std::ops::Drop for Deferred { @@ -216,30 +309,75 @@ impl std::ops::Drop for Deferred { } ``` -# Drawbacks -[drawbacks]: #drawbacks +### `TaskBuilder` -None? :grin: +`TaskBuilder` follows the [builder pattern](https://doc.rust-lang.org/1.0.0/style/ownership/builders.html) for constructing and scheduling a task. -# Rationale and alternatives -[alternatives]: #alternatives +```rust +pub struct TaskBuilder<'cx, C, E> { + // Hold a reference to `Context` so thunks do not require it as an argument + cx: &'cx mut C, + // Callback to execute on the libuv threadpool; complete is provided as part + // of the thunk + execute: E, +} -## High Level Promise Tasks +impl<'a: 'cx, 'cx, C, O, E> TaskBuilder<'cx, C, E> +where + C: Context<'a>, + O: Send + 'static, + E: FnOnce() -> O + Send + 'static, +{ + /// Constructs a new `TaskBuilder` + /// + /// See [`Context::task`] for more idiomatic usage + pub fn new(cx: &'cx mut C, execute: E) -> Self { + Self { cx, execute } + } -Using `JsPromise` along with other async features requires careful and verbose usage of several features (`JsPromise`, `EventQueue` and `try_catch`). Neon could exclusively provide a high-level API similar to `Task` or the proposed `TaskBuilder`. + /// Creates a promise and schedules the task to be executed, resolving + /// the promise with the result of the callback + pub fn promise(self, complete: F) -> Handle<'a, JsPromise> + where + V: Value, + for<'b> F: FnOnce(&mut TaskContext<'b>, O) -> JsResult<'b, V> + Send + 'static, + { + todo!() + } -```rust -fn async_task(cx: FunctionContext) -> JsResult { - let promise = cx.task(|| /* perform async task */) - .complete(|cx| /* convert to js types */); + /// Schedules the task to be executed, invoking the `complete` callback from + /// the JavaScript main thread with the result of `execute` + pub fn complete(self, complete: F) + where + F: FnOnce(&mut TaskContext, O) -> NeonResult<()> + Send + 'static, + { + todo!() + } +} - Ok(promise) +trait Context<'a> { + fn task<'cx, O, E>(&'cx mut self, execute: E) -> TaskBuilder + where + 'a: 'cx, + O: Send + 'static, + E: FnOnce() -> O + Send + 'static, + { + TaskBuilder::new(self, execute) + } } ``` -This API is very ergonomic, but removes a considerable amount of flexibility and power from the user. Instead, the initial implementation will focus on orthogonal core primitives like `Persistent`, `EventQueue`, `JsBox` and `JsPromise` that can later be combined into high-level APIs. +Internally, `TaskBuilder` uses [`napi_async_work`](https://nodejs.org/api/n-api.html#n_api_simple_asynchronous_operations) for creating and scheduling tasks. + +## Drawbacks +[drawbacks]: #drawbacks + +The most significant drawback is that the user is presented with many more options for asynchronous programming without clear direction on which to use. Neon will need to lean on documentation to guide users to the correct APIs for various situations. + +## Rationale and alternatives +[alternatives]: #alternatives -## Common `Deferred` pattern +### Common `Deferred` pattern Most promise libraries that provide a `Deferred` object provide the `promise` as part of that object. Neon might have a similar approach: @@ -254,7 +392,25 @@ The issue with this approach is that `Deferred` is `Send` and `JsPromise` is *no Instead a tuple is returned, similar to `(sender, receiver)` returned from `std::sync::mpsc::channel`. -# Unresolved questions +### Low level async work + +Neon could provide a lower level `AsyncWork` or `trait` that only accepts static function pointers and `&mut data` instead of consuming data. However, this would push complexity down for threading consumable state, try/catch and several other subtle issues. It is also boilerplate heavy. + +### Catching panics + +A `panic` in a `TaskBuilder::promise` will drop the promise without resolution. We could catch the unwind and convert to a rejection; however, catching panics is not considered best practice. Since we do not require catching unwind to prevent unsound unwinding across FFI, it's better to have a course grained control where the `Deferred` rejects when it drops without being settled. + +## Future Expansion + +Node-API allows async work to be [cancelled](https://nodejs.org/api/n-api.html#n_api_napi_cancel_async_work) after it has been scheduled, but before it has executed. This is left as future improvement since we do not have a clear use case in mind and it can be added without breaking changes. + +Two patterns for adding it without breaking change: +* Add `fn handle(&self) -> TaskHandle` to `TaskBuilder` to allow creating cancellation handles before scheduling +* Add methods that return a cancellation handle in addition to the thunk (e.g., `fn cancellable_promise(self, f: F) -> (TaskHandle, Handle)` + +## Unresolved questions [unresolved]: #unresolved-questions -- Should there be a constructor method on `JsPromise`? This is slightly awkward because it's impossible to create a `Promise` without also creating a `Deferred`. +* Are the thunk names `TaskBuilder::promise` and `TaskBuilder::complete` clear? +* Is it helpful to mirror deferred constructors across all types instead of only having `cx.promise()`? +* Is `cx.promise()` a good name or would `cx.deferred()` be better? From e918a52c7c38b3efb02a3789bccffc8ee02f8613 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Fri, 10 Sep 2021 09:06:17 -0400 Subject: [PATCH 3/8] Add Channel::send_and_settle --- text/0000-promise.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/text/0000-promise.md b/text/0000-promise.md index da183d2..fa38ffd 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -309,6 +309,37 @@ impl std::ops::Drop for Deferred { } ``` +#### `Channel` extension + +A new method `Channel::send_and_settle` is added to reduce the boilerplate of nested closures from `Channel::send` and `Deferred::settle_with`. Example without the convenience method: + +```rust +channel.send(move |mut cx| { + deferred.settle_with(&mut cx, |cx| Ok(cx.undefined())); + Ok(()) +}); +``` + +Example with `Channe::send_and_settle`: + +```rust +channel.send_and_settle(deferred, |cx| Ok(cx.undefined())); +``` + +```rust +impl Channel { + /// Settle a deferred promise with the value returned from a closure or the + /// exception thrown + pub fn send_and_settle(&self, f: F) + where + V: Value, + for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, + { + todo!() + } +} +``` + ### `TaskBuilder` `TaskBuilder` follows the [builder pattern](https://doc.rust-lang.org/1.0.0/style/ownership/builders.html) for constructing and scheduling a task. From 45e803424abacd1603e610ff6943a86c5258e354 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Fri, 10 Sep 2021 13:14:43 -0400 Subject: [PATCH 4/8] Fix typo and add try_send_and_settle --- text/0000-promise.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/text/0000-promise.md b/text/0000-promise.md index fa38ffd..4a25878 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -320,7 +320,7 @@ channel.send(move |mut cx| { }); ``` -Example with `Channe::send_and_settle`: +Example with `Channel::send_and_settle`: ```rust channel.send_and_settle(deferred, |cx| Ok(cx.undefined())); @@ -330,6 +330,8 @@ channel.send_and_settle(deferred, |cx| Ok(cx.undefined())); impl Channel { /// Settle a deferred promise with the value returned from a closure or the /// exception thrown + /// + /// Panics if sending fails pub fn send_and_settle(&self, f: F) where V: Value, @@ -337,6 +339,16 @@ impl Channel { { todo!() } + + /// Settle a deferred promise with the value returned from a closure or the + /// exception thrown + pub fn try_send_and_settle(&self, f: F) -> Result<(), SendError> + where + V: Value, + for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, + { + todo!() + } } ``` From ef9aedb9a037669ea85e37e390a0d271f5755284 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Fri, 10 Sep 2021 13:18:20 -0400 Subject: [PATCH 5/8] Add question --- text/0000-promise.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-promise.md b/text/0000-promise.md index 4a25878..57fd18a 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -457,3 +457,4 @@ Two patterns for adding it without breaking change: * Are the thunk names `TaskBuilder::promise` and `TaskBuilder::complete` clear? * Is it helpful to mirror deferred constructors across all types instead of only having `cx.promise()`? * Is `cx.promise()` a good name or would `cx.deferred()` be better? +* Is `Channel::send_and_settle` a good name? Is this API valuable or is the nested closure acceptable and clearer? From b56c2c95daf2bbec6a0890ea74ea96f6f2e7a4af Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Mon, 13 Sep 2021 13:09:08 -0400 Subject: [PATCH 6/8] Update from review --- text/0000-promise.md | 52 ++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/text/0000-promise.md b/text/0000-promise.md index 57fd18a..9bacaea 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -6,8 +6,6 @@ ## Summary [summary]: #summary -Provides an ergonomic API for executing tasks on the libuv threadpool and notifying JavaScript on the main thread with a result. - This RFC provides for two related, but distinct features: `JsPromise` and `TaskBuilder`. Since users will frequently be interacting with both, designing the two in tandem will result in a better design. ### `JsPromise` @@ -31,7 +29,7 @@ Provide an API for executing Rust closures on the libuv threadpool. ```rust cx.task(|| { /* Executes asynchronously on the threadpool */ }) - .complete(|cx, result| { + .and_then(|cx, result| { // Executes on the main JavaScript thread Ok(()) }); @@ -176,7 +174,7 @@ Tasks are asynchronously executed units of work performed on the libuv threadpoo * An `execute` callback that runs on the libuv threadpool * A `complete` callback that receives the result of `execute` and runs on the main JavaScript thread -The `execute` code is typically the slow _native_ operation asynchronously executed, while `complete` handles converting back to JavaScript values and resuming JavaScript execution with the result. +The `execute` code is typically the slow _native_ operation asynchronously executed, while `and_then` handles converting back to JavaScript values and resuming JavaScript execution with the result. For example, consider the previous `JsPromise` example that executes on a Rust thread. A similar result can be achieved by running on the libuv threadpool instead: @@ -185,7 +183,7 @@ fn async_method(cx: FunctionContext) -> JsResult { let (deferred, promise) = cx.promise(); cx.task(|| std::thread::sleep(std::time::Duration::from_millis(5000))) - .complete(move |cx, _result| { + .and_then(move |cx, _result| { let value = cx.undefined(); deferred.resolve(&mut cx, value); Ok(()) @@ -207,7 +205,7 @@ fn async_method(cx: FunctionContext) -> JsResult { } ``` -`TaskBuilder::complete` and `TaskBuilder::promise` are both thunks that consume the builder, creating and queueing the task. +`TaskBuilder::and_then` and `TaskBuilder::promise` are both thunks that consume the builder, creating and queueing the task. ## Reference-level explanation [reference-level-explanation]: #reference-level-explanation @@ -219,24 +217,12 @@ The `JsPromise` API consists of two structs: * `JsPromise`. Opaque value; only useful for passing back to JavaScript. * `Deferred`. Handle for resolving and rejecting the related `Promise`. -They are most idiomatically crated by `cx.promise()`, but construction is mirrored across `JsPromise` and `Deferred`. +`JsPromise` may only be constructed directly with `cx.promise()` instead of with `JsPromise::new` or `Deferred::new`. This is because they may not be constructed independently and it follows the convetion of `std::sync::mpsc::channel`. ```rust trait Context { fn promise(&mut self) -> (Deferred, Handle); } - -impl JsPromise { - pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { - todo!() - } -} - -impl Deferred { - pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { - todo!() - } -} ``` #### `JsPromise` @@ -264,12 +250,6 @@ impl ValueInternal for JsPromise { } impl Object for JsPromise {} - -impl JsPromise { - pub fn new<'a, C: Context<'a>>(cx: &mut C) -> (Deferred, Handle<'a, JsPromise>) { - todo!() - } -} ``` #### `Deferred` @@ -311,7 +291,7 @@ impl std::ops::Drop for Deferred { #### `Channel` extension -A new method `Channel::send_and_settle` is added to reduce the boilerplate of nested closures from `Channel::send` and `Deferred::settle_with`. Example without the convenience method: +A new method `Channel::settle_with` is added to reduce the boilerplate of nested closures from `Channel::send` and `Deferred::settle_with`. Example without the convenience method: ```rust channel.send(move |mut cx| { @@ -320,10 +300,10 @@ channel.send(move |mut cx| { }); ``` -Example with `Channel::send_and_settle`: +Example with `Channel::settle_with`: ```rust -channel.send_and_settle(deferred, |cx| Ok(cx.undefined())); +channel.settle_with(deferred, |cx| Ok(cx.undefined())); ``` ```rust @@ -332,7 +312,7 @@ impl Channel { /// exception thrown /// /// Panics if sending fails - pub fn send_and_settle(&self, f: F) + pub fn settle_with(&self, f: F) where V: Value, for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, @@ -342,7 +322,7 @@ impl Channel { /// Settle a deferred promise with the value returned from a closure or the /// exception thrown - pub fn try_send_and_settle(&self, f: F) -> Result<(), SendError> + pub fn try_settle_with(&self, f: F) -> Result<(), SendError> where V: Value, for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, @@ -388,9 +368,9 @@ where todo!() } - /// Schedules the task to be executed, invoking the `complete` callback from + /// Schedules the task to be executed, invoking the `and_then` callback from /// the JavaScript main thread with the result of `execute` - pub fn complete(self, complete: F) + pub fn and_then(self, complete: F) where F: FnOnce(&mut TaskContext, O) -> NeonResult<()> + Send + 'static, { @@ -454,7 +434,7 @@ Two patterns for adding it without breaking change: ## Unresolved questions [unresolved]: #unresolved-questions -* Are the thunk names `TaskBuilder::promise` and `TaskBuilder::complete` clear? -* Is it helpful to mirror deferred constructors across all types instead of only having `cx.promise()`? -* Is `cx.promise()` a good name or would `cx.deferred()` be better? -* Is `Channel::send_and_settle` a good name? Is this API valuable or is the nested closure acceptable and clearer? +* ~~Are the thunk names `TaskBuilder::promise` and `TaskBuilder::complete` clear?~~ `TaskBuilder::and_then` is a better match for Rust naming conventions. +* ~~Is it helpful to mirror deferred constructors across all types instead of only having `cx.promise()`?~~ No, they will be removed to match `std::sync::mpsc::channel`. +* ~~Is `cx.promise()` a good name or would `cx.deferred()` be better?~~ `cx.promise()` is better because `JsPromise` is the primary thing wanted and `Deferred` is an implementation detail. +* ~~Is `Channel::send_and_settle` a good name? Is this API valuable or is the nested closure acceptable and clearer?~~ `Channel::settle_with` helps to describe the function by matching `Deferred::settle_with`. From b12d9d5d33bd00386bfe994f5d16a1df3c59d8aa Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Mon, 13 Sep 2021 17:55:30 -0400 Subject: [PATCH 7/8] Add JoinHandle return types to channel methods --- text/0000-promise.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-promise.md b/text/0000-promise.md index 9bacaea..43ac82c 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -312,7 +312,7 @@ impl Channel { /// exception thrown /// /// Panics if sending fails - pub fn settle_with(&self, f: F) + pub fn settle_with(&self, f: F) -> JoinHandle<()> where V: Value, for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, @@ -322,7 +322,7 @@ impl Channel { /// Settle a deferred promise with the value returned from a closure or the /// exception thrown - pub fn try_settle_with(&self, f: F) -> Result<(), SendError> + pub fn try_settle_with(&self, f: F) -> Result, SendError> where V: Value, for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, From f88d46ca795e1988eca605576bd30dd4fd306fd5 Mon Sep 17 00:00:00 2001 From: "K.J. Valencik" Date: Fri, 22 Oct 2021 10:01:50 -0400 Subject: [PATCH 8/8] Updates to handle panics, exceptions and owned contexts --- text/0000-promise.md | 119 +++++++++++++++++++++---------------------- 1 file changed, 57 insertions(+), 62 deletions(-) diff --git a/text/0000-promise.md b/text/0000-promise.md index 43ac82c..259aaaf 100644 --- a/text/0000-promise.md +++ b/text/0000-promise.md @@ -159,12 +159,10 @@ fn async_method(cx: FunctionContext) -> JsResult { } ``` -However, there are still some subtle gotchas. What if there is a JavaScript exception? What if I don't resolve the `Deferred`? To help, `Deferred::settle_with` is provided as a helper that combines `cx.try_catch(...)` with `Deferred` resolution or rejection: +However, there are still some subtle gotchas. What if there is a JavaScript exception? What if I don't resolve the `Deferred`? To help, `Deferred::settle_with` is provided as a helper that combines `cx.try_catch(...)` and `Channel::send` with `Deferred` resolution or rejection: ```rust -channel.send(move |mut cx| { - deferred.settle_with(&mut cx, |cx| Ok(cx.undefined())); -}); +deferred.settle_with(&channel, |mut cx| Ok(cx.undefined())); ``` ### `TaskBuilder` @@ -183,7 +181,7 @@ fn async_method(cx: FunctionContext) -> JsResult { let (deferred, promise) = cx.promise(); cx.task(|| std::thread::sleep(std::time::Duration::from_millis(5000))) - .and_then(move |cx, _result| { + .and_then(move |mut cx, _result| { let value = cx.undefined(); deferred.resolve(&mut cx, value); Ok(()) @@ -199,7 +197,7 @@ However, since most of the time tasks will complete by settling a promise, a con fn async_method(cx: FunctionContext) -> JsResult { let promise = cx .task(|| std::thread::sleep(std::time::Duration::from_millis(5000))) - .promise(|cx, _result| Ok(cx.undefined())); + .promise(|mut cx, _result| Ok(cx.undefined())); Ok(promise) } @@ -217,7 +215,7 @@ The `JsPromise` API consists of two structs: * `JsPromise`. Opaque value; only useful for passing back to JavaScript. * `Deferred`. Handle for resolving and rejecting the related `Promise`. -`JsPromise` may only be constructed directly with `cx.promise()` instead of with `JsPromise::new` or `Deferred::new`. This is because they may not be constructed independently and it follows the convetion of `std::sync::mpsc::channel`. +`JsPromise` may only be constructed directly with `cx.promise()` instead of with `JsPromise::new` or `Deferred::new`. This is because they may not be constructed independently, and it follows the convention of `std::sync::mpsc::channel`. ```rust trait Context { @@ -270,16 +268,25 @@ impl Deferred { /// Rejects a deferred Promise with the error pub fn reject<'a, E: Value, C: Context<'a>>(self, cx: &mut C, err: Handle); - /// Resolves or rejects a Promise with the result of a closure - pub fn settle_with<'a, T, C, F>(self, cx: &mut C, f: F) + /// Attempts to resolve a promise with the result of a closure sent across a closure + pub fn try_settle_with( + self, + channel: &Channel, + complete: F, + ) -> Result, SendError> where - T: Value, - C: Context<'a>, - F: FnOnce(&mut C) -> JsResult<'a, T>; + V: Value, + F: FnOnce(TaskContext) -> JsResult + Send + 'static; + + /// Same as `Deferred::try_settle_with`, but panics if the closure could not be sent + pub fn settle_with(self, channel: &Channel, complete: F) -> JoinHandle<()> + where + V: Value, + F: FnOnce(TaskContext) -> JsResult + Send + 'static; } ``` -Similar to [`Root`](https://github.com/neon-bindings/rfcs/pull/32), if a `Deferred` is not resolved or rejected, the `Promise` chain will leak. To help catch this and guide users towards a correct implementation, `Deferred` should `panic` on `Drop` if not used on Node-API < 4 and be dropped on a global drop queue in Node-API >= 4. +Similar to [`Root`](https://github.com/neon-bindings/rfcs/pull/32), if a `Deferred` is not resolved or rejected, the `Promise` chain will leak. To help catch this and guide users towards a correct implementation, `Deferred` should `panic` on `Drop` if used on Node-API < 4 and be dropped on a global drop queue in Node-API >= 4, rejecting the promise. ```rust impl std::ops::Drop for Deferred { @@ -289,49 +296,6 @@ impl std::ops::Drop for Deferred { } ``` -#### `Channel` extension - -A new method `Channel::settle_with` is added to reduce the boilerplate of nested closures from `Channel::send` and `Deferred::settle_with`. Example without the convenience method: - -```rust -channel.send(move |mut cx| { - deferred.settle_with(&mut cx, |cx| Ok(cx.undefined())); - Ok(()) -}); -``` - -Example with `Channel::settle_with`: - -```rust -channel.settle_with(deferred, |cx| Ok(cx.undefined())); -``` - -```rust -impl Channel { - /// Settle a deferred promise with the value returned from a closure or the - /// exception thrown - /// - /// Panics if sending fails - pub fn settle_with(&self, f: F) -> JoinHandle<()> - where - V: Value, - for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, - { - todo!() - } - - /// Settle a deferred promise with the value returned from a closure or the - /// exception thrown - pub fn try_settle_with(&self, f: F) -> Result, SendError> - where - V: Value, - for<'a> F: FnOnce(&mut TaskContext<'a>) -> JsResult<'a, Value> + Send + 'static, - { - todo!() - } -} -``` - ### `TaskBuilder` `TaskBuilder` follows the [builder pattern](https://doc.rust-lang.org/1.0.0/style/ownership/builders.html) for constructing and scheduling a task. @@ -363,16 +327,18 @@ where pub fn promise(self, complete: F) -> Handle<'a, JsPromise> where V: Value, - for<'b> F: FnOnce(&mut TaskContext<'b>, O) -> JsResult<'b, V> + Send + 'static, + F: FnOnce(TaskContext, O) -> JsResult + Send + 'static, { todo!() } /// Schedules the task to be executed, invoking the `and_then` callback from /// the JavaScript main thread with the result of `execute` + /// + /// If the `task` panics, the `Err` will contain the value the thread panicked with pub fn and_then(self, complete: F) where - F: FnOnce(&mut TaskContext, O) -> NeonResult<()> + Send + 'static, + F: FnOnce(TaskContext, std::thread::Result) -> NeonResult<()> + Send + 'static, { todo!() } @@ -392,6 +358,39 @@ trait Context<'a> { Internally, `TaskBuilder` uses [`napi_async_work`](https://nodejs.org/api/n-api.html#n_api_simple_asynchronous_operations) for creating and scheduling tasks. +### Catching panics and exceptions + +Panics and exceptions are handled both in tasks that use promises and ones that do not. + +When using `TaskBuilder::promise`, the promise is rejected with the error. When using `TaskBuilder::and_then`, the error is passed as an `uncaughtException` on Node-API >= 3. On Node-API 1 and 2, the process is terminated with an error message. + +The following table describes the action taken and the shape of the error. + +| Task Thunk | Action | Exception | Panic | Both | +| ---------- | ------------------- | ---------------- | ---------------- | ---------------- | +| `and_then` | `uncaughtException` | `NeonAsyncError` | `NeonAsyncError` | `NeonAsyncError` | +| `promise` | `Promise.reject` | `any` | `NeonAsyncError` | `NeonAsyncError` | + +```ts +interface NeonAsyncError extends Error { + // General message describing the error that occurred + message: string; + // If an exception occurred, the value that was thrown + cause?: any; + // If a panic ocurred, an Error representing the panic + panic?: PanicError; +} + +interface PanicError extends Error { + // Panic message if it could be downcast to a String, otherwise generic message + message: string; + // If the panic could not be downcast as a String, the panic in a `JsBox` + cause?: JsBox +} +``` + +Additionally, `Channel::send` and `Channel::try_send` are updated to behave identically to `TaskBuilder::and_then` when a panic or exception is thrown. + ## Drawbacks [drawbacks]: #drawbacks @@ -419,10 +418,6 @@ Instead a tuple is returned, similar to `(sender, receiver)` returned from `std: Neon could provide a lower level `AsyncWork` or `trait` that only accepts static function pointers and `&mut data` instead of consuming data. However, this would push complexity down for threading consumable state, try/catch and several other subtle issues. It is also boilerplate heavy. -### Catching panics - -A `panic` in a `TaskBuilder::promise` will drop the promise without resolution. We could catch the unwind and convert to a rejection; however, catching panics is not considered best practice. Since we do not require catching unwind to prevent unsound unwinding across FFI, it's better to have a course grained control where the `Deferred` rejects when it drops without being settled. - ## Future Expansion Node-API allows async work to be [cancelled](https://nodejs.org/api/n-api.html#n_api_napi_cancel_async_work) after it has been scheduled, but before it has executed. This is left as future improvement since we do not have a clear use case in mind and it can be added without breaking changes.