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

V3: Include stack traces for napi threadsafe functions #101

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 43 additions & 59 deletions crates/atlaspack_napi_helpers/src/js_callable/js_callable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use napi::Env;
use napi::JsFunction;
use napi::JsObject;
use napi::JsUnknown;
use napi::ValueType;
use serde::de::DeserializeOwned;
use serde::Serialize;

Expand All @@ -18,7 +19,7 @@ use super::map_return_serde;
use super::JsValue;

pub type MapJsParams = Box<dyn FnOnce(&Env) -> napi::Result<Vec<JsUnknown>> + 'static>;
pub type MapJsReturn<Return> = Box<dyn Fn(&Env, JsUnknown) -> napi::Result<Return> + 'static>;
pub type MapJsReturn<Return> = Box<dyn Fn(&Env, JsUnknown) -> anyhow::Result<Return> + 'static>;

/// JsCallable provides a Send + Sync wrapper around callable JavaScript functions
///
Expand Down Expand Up @@ -78,90 +79,73 @@ impl JsCallable {
Ok(self)
}

/// Call JavaScript function and discard return value
pub fn call(
&self,
map_params: impl FnOnce(&Env) -> napi::Result<Vec<JsUnknown>> + 'static,
) -> napi::Result<()> {
#[cfg(debug_assertions)]
if self.initial_thread == std::thread::current().id() {
return Err(napi::Error::from_reason(format!(
"Cannot run threadsafe function {} on main thread",
self.name
)));
}

self
.threadsafe_function
.call(Box::new(map_params), ThreadsafeFunctionCallMode::Blocking);

Ok(())
}

pub fn call_serde<Params>(&self, params: Params) -> napi::Result<()>
where
Params: Serialize + Send + Sync + 'static,
{
self.call(map_params_serde(params))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were not using this and it has footguns so I thought it was best to remove it


/// Call JavaScript function and handle the return value
pub fn call_with_return<Return>(
pub fn call<Return>(
&self,
map_params: impl FnOnce(&Env) -> napi::Result<Vec<JsUnknown>> + 'static,
map_return: impl Fn(&Env, JsUnknown) -> napi::Result<Return> + 'static,
) -> napi::Result<Return>
map_return: impl Fn(&Env, JsUnknown) -> anyhow::Result<Return> + 'static,
) -> anyhow::Result<Return>
where
Return: Send + 'static,
{
#[cfg(debug_assertions)]
if self.initial_thread == std::thread::current().id() {
return Err(napi::Error::from_reason(format!(
anyhow::bail!(
"Cannot run threadsafe function {} on main thread",
self.name
)));
);
}

let (tx, rx) = channel();
let (tx, rx) = channel::<anyhow::Result<Return>>();

self.threadsafe_function.call_with_return_value(
Box::new(map_params),
ThreadsafeFunctionCallMode::NonBlocking,
move |JsValue(value, env)| {
if value.is_promise()? {
let result: JsObject = value.try_into()?;
let then: JsFunction = result.get_named_property("then")?;

let tx2 = tx.clone();
let cb = env.create_function_from_closure("callback", move |ctx| {
tx.send(map_return(&env, ctx.get::<JsUnknown>(0)?)).unwrap();
ctx.env.get_undefined()
})?;

let eb = env.create_function_from_closure("error_callback", move |ctx| {
let err = napi::Error::from(ctx.get::<JsUnknown>(0)?);
tx2.send(Err(err)).expect("send failure");
ctx.env.get_undefined()
})?;

then.call(Some(&result), &[cb, eb])?;
} else if value.is_error()? {
tx.send(Err(napi::Error::from(value))).unwrap();
} else {
tx.send(map_return(&env, value)).unwrap();
}
let container: JsObject = value.try_into()?;
let then_fn: JsFunction = match container.get_named_property("then") {
Ok(then_fn) => then_fn,
Err(error) => {
tx.send(Err(anyhow::anyhow!(error))).unwrap();
return Ok(());
}
};

let then_callback = env.create_function_from_closure("callback", move |ctx| {
// Return
// [error, null]
// [null, value]
let result = ctx.get::<JsObject>(0)?;
let error = result.get_element::<JsUnknown>(0)?;

if let ValueType::Null = error.get_type()? {
let value = result.get_element::<JsUnknown>(1)?;
tx.send(map_return(&env, value)).unwrap();
} else {
let error_value = env.from_js_value::<String, JsUnknown>(error)?;
tx.send(Err(anyhow::anyhow!(error_value))).unwrap();
}

ctx.env.get_undefined()
})?;

then_fn.call(Some(&container), &[then_callback])?;
Ok(())
},
);

rx.recv().unwrap()
let result = rx.recv().unwrap();
match result {
Ok(value) => Ok(value),
Err(error) => Err(anyhow::anyhow!("{} {}", self.name, error)),
}
}

pub fn call_with_return_serde<Params, Return>(&self, params: Params) -> napi::Result<Return>
pub fn call_serde<Params, Return>(&self, params: Params) -> anyhow::Result<Return>
where
Params: Serialize + Send + Sync + 'static,
Return: Send + DeserializeOwned + 'static,
{
self.call_with_return(map_params_serde(params), map_return_serde())
self.call(map_params_serde(params), map_return_serde())
}
}
5 changes: 4 additions & 1 deletion crates/atlaspack_napi_helpers/src/js_callable/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,8 @@ pub fn map_return_serde<Return>() -> MapJsReturn<Return>
where
Return: Send + DeserializeOwned + 'static,
{
Box::new(move |env, value| env.from_js_value(&value))
Box::new(move |env, value| match env.from_js_value(&value) {
Ok(result) => Ok(result),
Err(err) => Err(anyhow::anyhow!(err.to_string())),
})
}
27 changes: 7 additions & 20 deletions crates/atlaspack_plugin_rpc/src/nodejs/rpc_conn_nodejs.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use atlaspack_core::plugin::Resolved;
use atlaspack_napi_helpers::anyhow_from_napi;
use atlaspack_napi_helpers::js_callable::JsCallable;
use napi::{JsObject, JsUnknown};

Expand Down Expand Up @@ -32,34 +31,22 @@ impl NodejsWorker {

impl RpcWorker for NodejsWorker {
fn ping(&self) -> anyhow::Result<()> {
self
.ping_fn
.call_with_return(
|_env| Ok(Vec::<JsUnknown>::new()),
|_env, _| Ok(Vec::<()>::new()),
)
.map_err(anyhow_from_napi)?;
self.ping_fn.call(
|_env| Ok(Vec::<JsUnknown>::new()),
|_env, _| Ok(Vec::<()>::new()),
)?;
Ok(())
}

fn load_plugin(&self, opts: LoadPluginOptions) -> anyhow::Result<()> {
self
.load_plugin_fn
.call_with_return_serde(opts)
.map_err(anyhow_from_napi)
self.load_plugin_fn.call_serde(opts)
}

fn run_resolver_resolve(&self, opts: RunResolverResolve) -> anyhow::Result<Resolved> {
self
.run_resolver_resolve_fn
.call_with_return_serde(opts)
.map_err(anyhow_from_napi)
self.run_resolver_resolve_fn.call_serde(opts)
}

fn run_transformer(&self, opts: RpcTransformerOpts) -> anyhow::Result<RpcTransformerResult> {
self
.transformer_register_fn
.call_with_return_serde(opts)
.map_err(anyhow_from_napi)
self.transformer_register_fn.call_serde(opts)
}
}
14 changes: 7 additions & 7 deletions crates/node-bindings/src/atlaspack/file_system_napi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,28 +40,28 @@ impl FileSystem for FileSystemNapi {
fn canonicalize_base(&self, path: &Path) -> io::Result<PathBuf> {
self
.canonicalize_fn
.call_with_return_serde(path.to_path_buf())
.call_serde(path.to_path_buf())
.map_err(|e| io::Error::other(e))
}

fn create_directory(&self, path: &Path) -> std::io::Result<()> {
self
.create_directory_fn
.call_with_return_serde(path.to_path_buf())
.call_serde(path.to_path_buf())
.map_err(|e| io::Error::other(e))
}

fn cwd(&self) -> io::Result<PathBuf> {
self
.cwd_fn
.call_with_return_serde(None::<bool>)
.call_serde(None::<bool>)
.map_err(|e| io::Error::other(e))
}

fn read(&self, path: &Path) -> std::io::Result<Vec<u8>> {
let result = self
.read_file_fn
.call_with_return_serde(path.to_path_buf())
.call_serde(path.to_path_buf())
.map_err(|e| io::Error::other(e));

result
Expand All @@ -70,21 +70,21 @@ impl FileSystem for FileSystemNapi {
fn read_to_string(&self, path: &Path) -> io::Result<String> {
self
.read_file_fn
.call_with_return_serde((path.to_path_buf(), "utf8"))
.call_serde((path.to_path_buf(), "utf8"))
.map_err(|e| io::Error::other(e))
}

fn is_file(&self, path: &Path) -> bool {
self
.is_file_fn
.call_with_return_serde(path.to_path_buf())
.call_serde(path.to_path_buf())
.expect("TODO handle error case")
}

fn is_dir(&self, path: &Path) -> bool {
self
.is_dir_fn
.call_with_return_serde(path.to_path_buf())
.call_serde(path.to_path_buf())
.expect("TODO handle error case")
}
}
4 changes: 1 addition & 3 deletions crates/node-bindings/src/atlaspack/package_manager_napi.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::path::Path;

use anyhow::anyhow;
use napi::{Env, JsObject};

use atlaspack_napi_helpers::js_callable::JsCallable;
Expand All @@ -23,7 +22,6 @@ impl PackageManager for PackageManagerNapi {
fn resolve(&self, specifier: &str, from: &Path) -> anyhow::Result<Resolution> {
self
.resolve_fn
.call_with_return_serde((specifier.to_owned(), from.to_path_buf()))
.map_err(|e| anyhow!(e))
.call_serde((specifier.to_owned(), from.to_path_buf()))
}
}
19 changes: 12 additions & 7 deletions packages/core/core/src/atlaspack-v3/fs.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,46 @@
// @flow strict-local

// import type { JsCallable } from "./jsCallable";
import type {FileSystem} from '@atlaspack/rust';
import type {
Encoding,
FilePath,
FileSystem as ClassicFileSystem,
} from '@atlaspack/types-internal';
} from '@atlaspack/types';

import {jsCallable} from './jsCallable';

// Move to @atlaspack/utils or a dedicated v3 / migration package later
export function toFileSystemV3(fs: ClassicFileSystem): FileSystem {
return {
// $FlowFixMe migrate to TypeScript
canonicalize: jsCallable((path: FilePath) => fs.realpathSync(path)),
createDirectory: jsCallable((path: FilePath) => fs.mkdirp(path)),
// $FlowFixMe migrate to TypeScript
cwd: jsCallable(() => fs.cwd()),
// $FlowFixMe migrate to TypeScript
readFile: jsCallable((path: string, encoding?: Encoding) => {
if (!encoding) {
// $FlowFixMe
return [...fs.readFileSync(path)];
} else {
return fs.readFileSync(path, encoding);
}
return fs.readFileSync(path, encoding);
}),
isFile: (path: string) => {
// $FlowFixMe migrate to TypeScript
isFile: jsCallable((path: string) => {
try {
return fs.statSync(path).isFile();
} catch {
return false;
}
},
isDir: (path: string) => {
}),
// $FlowFixMe migrate to TypeScript
alshdavid marked this conversation as resolved.
Show resolved Hide resolved
isDir: jsCallable((path: string) => {
try {
return fs.statSync(path).isDirectory();
} catch {
return false;
}
},
}),
};
}
20 changes: 14 additions & 6 deletions packages/core/core/src/atlaspack-v3/jsCallable.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@

export type JsCallable<Args: $ReadOnlyArray<mixed>, Return> = (
...Args
) => Return;
) => JsCallableResult<Return>;

export type JsCallableResult<Return> = Promise<[string | null, Return | null]>;

export function jsCallable<Args: $ReadOnlyArray<mixed>, Return>(
fn: (...Args) => Return,
): (...Args) => Return {
return (...args: Args) => {
fn: (...Args) => Return | Promise<Return>,
): JsCallable<Args, Return> {
return async (...args) => {
try {
return fn(...args);
const result = await fn(...args);
return [null, result];
} catch (err) {
return err;
if (err instanceof Error) {
return [err.stack, null];
}
// $FlowFixMe migrate to TypeScript
let errStr = `${err}`;
return [`${errStr}`, null];
}
};
}
Loading
Loading