Skip to content

Commit 42eb50e

Browse files
mjameswhmjameswh
authored
fix(core): Do not retain static references to error constructors in rust bridge (#983)
1 parent 1ef7eb1 commit 42eb50e

20 files changed

+211
-181
lines changed

‎package-lock.json

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/core-bridge/index.js

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,31 @@
11
const { getPrebuiltPath } = require('./common');
2+
const typescriptExports = require('./lib/index');
3+
const { convertFromNamedError } = require('./lib/errors');
4+
5+
function wrapErrors(fn) {
6+
return (...args) => {
7+
try {
8+
// Some of our native functions expect callback functions. When present, these callbacks are
9+
// always the last argument passed to the function, and always adhere to the signature
10+
// `callback(err, result)`. If a callback is present, then make sure that errors sent
11+
// to it are also converted.
12+
if (typeof args[args.length - 1] === 'function') {
13+
const callback = args[args.length - 1];
14+
args[args.length - 1] = (e, x) => callback(convertFromNamedError(e, false), x);
15+
}
16+
return fn(...args);
17+
} catch (e) {
18+
throw convertFromNamedError(e, true);
19+
}
20+
};
21+
}
222

323
try {
4-
const prebuiltPath = getPrebuiltPath();
5-
module.exports = require(prebuiltPath);
24+
const nativeLibPath = getPrebuiltPath();
25+
const nativeExports = Object.fromEntries(
26+
Object.entries(require(nativeLibPath)).map(([name, fn]) => [name, wrapErrors(fn)])
27+
);
28+
module.exports = { ...typescriptExports, ...nativeExports };
629
} catch (err) {
730
throw err;
831
}

‎packages/core-bridge/package.json

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"version": "1.4.3",
44
"description": "Temporal.io SDK Core<>Node bridge",
55
"main": "index.js",
6-
"types": "index.d.ts",
6+
"types": "lib/index.d.ts",
77
"scripts": {
88
"build-rust": "node ./scripts/build.js --force",
99
"build": "npm run build-rust",
@@ -23,6 +23,7 @@
2323
"license": "MIT",
2424
"dependencies": {
2525
"@opentelemetry/api": "^1.1.0",
26+
"@temporalio/common": "file:../common",
2627
"arg": "^5.0.2",
2728
"cargo-cp-artifact": "^0.1.6",
2829
"which": "^2.0.2"
@@ -40,7 +41,9 @@
4041
"Cargo.lock",
4142
"index.js",
4243
"common.js",
43-
"index.d.ts"
44+
"index.d.ts",
45+
"ts",
46+
"lib"
4447
],
4548
"publishConfig": {
4649
"access": "public"

‎packages/core-bridge/src/errors.rs

Lines changed: 28 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -1,96 +1,38 @@
11
use neon::prelude::*;
2-
use once_cell::sync::OnceCell;
32

43
/// An unhandled error while communicating with the server, considered fatal
5-
pub static TRANSPORT_ERROR: OnceCell<Root<JsFunction>> = OnceCell::new();
4+
pub static TRANSPORT_ERROR: &str = "TransportError";
65
/// Thrown after shutdown was requested as a response to a poll function, JS should stop polling
76
/// once this error is encountered
8-
pub static SHUTDOWN_ERROR: OnceCell<Root<JsFunction>> = OnceCell::new();
7+
pub static SHUTDOWN_ERROR: &str = "ShutdownError";
98
/// Something unexpected happened, considered fatal
10-
pub static UNEXPECTED_ERROR: OnceCell<Root<JsFunction>> = OnceCell::new();
9+
pub static UNEXPECTED_ERROR: &str = "UnexpectedError";
1110
/// Used in different parts of the project to signal that something unexpected has happened
12-
pub static ILLEGAL_STATE_ERROR: OnceCell<Root<JsFunction>> = OnceCell::new();
13-
14-
static ALREADY_REGISTERED_ERRORS: OnceCell<bool> = OnceCell::new();
15-
16-
/// This is one of the ways to implement custom errors in neon.
17-
/// Taken from the answer in GitHub issues: https://github.com/neon-bindings/neon/issues/714
18-
pub trait CustomError {
19-
fn construct<'a, C>(&self, cx: &mut C, args: Vec<Handle<JsValue>>) -> JsResult<'a, JsObject>
20-
where
21-
C: Context<'a>;
22-
23-
fn construct_from_string<'a, C>(&self, cx: &mut C, message: impl Into<String>) -> JsResult<'a, JsObject>
24-
where
25-
C: Context<'a>;
26-
27-
fn construct_from_error<'a, C, E>(&self, cx: &mut C, err: E) -> JsResult<'a, JsObject>
28-
where
29-
C: Context<'a>,
30-
E: std::error::Error;
31-
}
32-
33-
// Implement `CustomError` for ALL errors in a `OnceCell`. This only needs to be
34-
// done _once_ even if other errors are added.
35-
impl CustomError for OnceCell<Root<JsFunction>> {
36-
fn construct<'a, C>(&self, cx: &mut C, args: Vec<Handle<JsValue>>) -> JsResult<'a, JsObject>
37-
where
38-
C: Context<'a>,
39-
{
40-
let error = self
41-
.get()
42-
.expect("Expected module to be initialized")
43-
.to_inner(cx);
44-
45-
// Use `.construct` to call this as a constructor instead of a normal function
46-
error.construct(cx, args)
47-
}
48-
49-
fn construct_from_string<'a, C>(&self, cx: &mut C, message: impl Into<String>) -> JsResult<'a, JsObject>
50-
where
51-
C: Context<'a>,
52-
{
53-
let args = vec![cx.string(message.into()).upcast()];
54-
self.construct(cx, args)
55-
}
56-
57-
fn construct_from_error<'a, C, E>(&self, cx: &mut C, err: E) -> JsResult<'a, JsObject>
58-
where
59-
C: Context<'a>,
60-
E: std::error::Error,
61-
{
62-
self.construct_from_string(cx, format!("{:?}", err))
63-
}
11+
pub static ILLEGAL_STATE_ERROR: &str = "IllegalStateError";
12+
13+
pub fn make_named_error_from_string<'a, C>(
14+
cx: &mut C,
15+
name: &str,
16+
message: impl Into<String>,
17+
) -> JsResult<'a, JsError>
18+
where
19+
C: Context<'a>,
20+
{
21+
let error = cx.error(message.into()).unwrap();
22+
let name = cx.string(name);
23+
error.set(cx, "name", name)?;
24+
25+
Ok(error)
6426
}
6527

66-
/// This method should be manually called _once_ from JavaScript to initialize the module
67-
/// It expects a single argument, an object with the various Error constructors.
68-
/// This is a very common pattern in Neon modules.
69-
pub fn register_errors(mut cx: FunctionContext) -> JsResult<JsUndefined> {
70-
let res = ALREADY_REGISTERED_ERRORS.set(true);
71-
if res.is_err() {
72-
// Don't do anything if errors are already registered
73-
return Ok(cx.undefined());
74-
}
75-
76-
let mapping = cx.argument::<JsObject>(0)?;
77-
let shutdown_error = mapping
78-
.get::<JsFunction, _, _>(&mut cx, "ShutdownError")?
79-
.root(&mut cx);
80-
let transport_error = mapping
81-
.get::<JsFunction, _, _>(&mut cx, "TransportError")?
82-
.root(&mut cx);
83-
let unexpected_error = mapping
84-
.get::<JsFunction, _, _>(&mut cx, "UnexpectedError")?
85-
.root(&mut cx);
86-
let illegal_state_error = mapping
87-
.get::<JsFunction, _, _>(&mut cx, "IllegalStateError")?
88-
.root(&mut cx);
89-
90-
TRANSPORT_ERROR.get_or_try_init(|| Ok(transport_error))?;
91-
SHUTDOWN_ERROR.get_or_try_init(|| Ok(shutdown_error))?;
92-
UNEXPECTED_ERROR.get_or_try_init(|| Ok(unexpected_error))?;
93-
ILLEGAL_STATE_ERROR.get_or_try_init(|| Ok(illegal_state_error))?;
94-
95-
Ok(cx.undefined())
28+
pub fn make_named_error_from_error<'a, C, E>(
29+
cx: &mut C,
30+
name: &str,
31+
err: E,
32+
) -> JsResult<'a, JsError>
33+
where
34+
C: Context<'a>,
35+
E: std::error::Error,
36+
{
37+
make_named_error_from_string(cx, name, format!("{:?}", err))
9638
}

‎packages/core-bridge/src/helpers.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ where
6262
{
6363
let err_str = format!("{}", err);
6464
callback_with_error(cx, callback, move |cx| {
65-
UNEXPECTED_ERROR.construct_from_string(cx, err_str)
65+
make_named_error_from_string(cx, UNEXPECTED_ERROR, err_str)
6666
})
6767
}
6868

‎packages/core-bridge/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use testing::*;
1313
#[neon::main]
1414
fn main(mut cx: ModuleContext) -> NeonResult<()> {
1515
cx.export_function("getTimeOfDay", get_time_of_day)?;
16-
cx.export_function("registerErrors", errors::register_errors)?;
1716
cx.export_function("newRuntime", runtime_new)?;
1817
cx.export_function("newClient", client_new)?;
1918
cx.export_function("clientUpdateHeaders", client_update_headers)?;

‎packages/core-bridge/src/runtime.rs

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use crate::{conversions::*, errors::*, helpers::*, worker::*};
2+
use neon::context::Context;
23
use neon::prelude::*;
34
use parking_lot::RwLock;
45
use std::cell::Cell;
@@ -154,16 +155,18 @@ pub fn start_bridge_loop(
154155
{
155156
Err(err) => {
156157
send_error(channel.clone(), callback, |cx| match err {
157-
ClientInitError::SystemInfoCallError(e) => TRANSPORT_ERROR
158-
.construct_from_string(
158+
ClientInitError::SystemInfoCallError(e) => {
159+
make_named_error_from_string(
159160
cx,
161+
TRANSPORT_ERROR,
160162
format!("Failed to call GetSystemInfo: {}", e),
161-
),
163+
)
164+
}
162165
ClientInitError::TonicTransportError(e) => {
163-
TRANSPORT_ERROR.construct_from_error(cx, e)
166+
make_named_error_from_error(cx, TRANSPORT_ERROR, e)
164167
}
165168
ClientInitError::InvalidUri(e) => {
166-
Ok(JsError::type_error(cx, format!("{}", e))?.upcast())
169+
Ok(JsError::type_error(cx, format!("{}", e))?)
167170
}
168171
});
169172
}
@@ -224,7 +227,7 @@ pub fn start_bridge_loop(
224227
});
225228
}
226229
Err(err) => send_error(channel.clone(), callback, move |cx| {
227-
UNEXPECTED_ERROR.construct_from_error(cx, err.deref())
230+
make_named_error_from_error(cx, UNEXPECTED_ERROR, err.deref())
228231
}),
229232
}
230233
}
@@ -253,7 +256,7 @@ pub fn start_bridge_loop(
253256
})
254257
}
255258
Err(err) => send_error(channel.clone(), callback, move |cx| {
256-
UNEXPECTED_ERROR.construct_from_error(cx, err.deref())
259+
make_named_error_from_error(cx, UNEXPECTED_ERROR, err.deref())
257260
}),
258261
};
259262
}
@@ -275,7 +278,7 @@ pub fn start_bridge_loop(
275278
Err(err) => {
276279
let err_str = format!("Failed to start ephemeral server: {}", err);
277280
send_error(channel.clone(), callback, |cx| {
278-
UNEXPECTED_ERROR.construct_from_string(cx, err_str)
281+
make_named_error_from_string(cx, UNEXPECTED_ERROR, err_str)
279282
});
280283
}
281284
Ok(server) => {
@@ -299,8 +302,9 @@ pub fn start_bridge_loop(
299302
guard.shutdown().await
300303
},
301304
|cx, err| {
302-
UNEXPECTED_ERROR.construct_from_string(
305+
make_named_error_from_string(
303306
cx,
307+
UNEXPECTED_ERROR,
304308
format!("Failed to start test server: {}", err),
305309
)
306310
},
@@ -323,8 +327,11 @@ pub fn start_bridge_loop(
323327
})
324328
};
325329
void_future_to_js(channel, callback, sendfut, |cx, err| {
326-
UNEXPECTED_ERROR
327-
.construct_from_string(cx, format!("Error pushing replay history {}", err))
330+
make_named_error_from_string(
331+
cx,
332+
UNEXPECTED_ERROR,
333+
format!("Error pushing replay history {}", err),
334+
)
328335
})
329336
.await
330337
});
@@ -436,8 +443,7 @@ pub fn client_new(mut cx: FunctionContext) -> JsResult<JsUndefined> {
436443
pub fn client_close(mut cx: FunctionContext) -> JsResult<JsUndefined> {
437444
let client = cx.argument::<BoxedClient>(0)?;
438445
if client.replace(None).is_none() {
439-
ILLEGAL_STATE_ERROR
440-
.construct_from_string(&mut cx, "Client already closed")
446+
make_named_error_from_string(&mut cx, ILLEGAL_STATE_ERROR, "Client already closed")
441447
.and_then(|err| cx.throw(err))?;
442448
};
443449
Ok(cx.undefined())

‎packages/core-bridge/src/testing.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,12 @@ pub fn get_ephemeral_server_target(mut cx: FunctionContext) -> JsResult<JsString
3434
.as_ref()
3535
.map(|s| cx.string(s.core_server.blocking_lock().target.as_str()));
3636
if target.is_none() {
37-
ILLEGAL_STATE_ERROR
38-
.construct_from_string(&mut cx, "Tried to use closed test server")
39-
.and_then(|err| cx.throw(err))?;
37+
make_named_error_from_string(
38+
&mut cx,
39+
ILLEGAL_STATE_ERROR,
40+
"Tried to use closed test server",
41+
)
42+
.and_then(|err| cx.throw(err))?;
4043
};
4144
Ok(target.unwrap())
4245
}

0 commit comments

Comments
 (0)