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

Fix panic during shutdown when using channel #934

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion crates/neon/src/event/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,9 @@ impl Drop for Channel {
// UV thread if strong reference count goes to 0.
let state = Arc::clone(&self.state);

self.send(move |mut cx| {
// Use `try_send` instead of `send` because, if `send` fails, it means we're shutting down
// the VM and don't need to worry about cleanup.
let _ = self.try_send(move |mut cx| {
state.unref(&mut cx);
Ok(())
});
Expand Down
19 changes: 16 additions & 3 deletions test/napi/lib/workers.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const {
isMainThread,
parentPort,
threadId,
workerData,
} = require("worker_threads");

const addon = require("..");
Expand All @@ -15,7 +16,11 @@ if (!isMainThread) {
// succeed even if the presence of a bug.
addon.reject_after(new Error("Oh, no!"), 200).catch(() => {});

// Reproduce another shutdown bug; this one isn't timing-dependent.
let boxed_channels = addon.box_channels();

addon.get_or_init_thread_id(threadId);

parentPort.once("message", (message) => {
try {
switch (message) {
Expand Down Expand Up @@ -44,6 +49,10 @@ if (!isMainThread) {
}
});

if (workerData === "notify_when_startup_complete") {
parentPort.postMessage("startup_complete");
}

return;
}

Expand Down Expand Up @@ -194,9 +203,13 @@ describe("Instance-local storage", () => {
});

it("should be able to exit a worker without a crash", (cb) => {
const worker = new Worker(__filename);
const worker = new Worker(__filename, {
workerData: "notify_when_startup_complete",
});

setTimeout(() => worker.terminate(), 50);
setTimeout(cb, 100);
worker.once("message", async () => {
await worker.terminate();
cb();
Copy link
Member

@kjvalencik kjvalencik Nov 8, 2022

Choose a reason for hiding this comment

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

I believe this still needs to be delayed a bit for the race, but to be honest, I don't exactly remember the ordering of this test.

});
});
});
17 changes: 17 additions & 0 deletions test/napi/src/js/workers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,20 @@ pub fn reject_after(mut cx: FunctionContext) -> JsResult<JsPromise> {

Ok(promise)
}

#[allow(dead_code)]
pub struct Channels {
channel_1: Channel,
channel_2: Channel,
}
impl Finalize for Channels {}

pub fn box_channels(mut cx: FunctionContext) -> JsResult<JsBox<Channels>> {
let channel_1 = cx.channel();
let channel_2 = cx.channel();

Ok(cx.boxed(Channels {
channel_1,
channel_2,
}))
}
1 change: 1 addition & 0 deletions test/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> {
cx.export_function("stash_global_object", js::workers::stash_global_object)?;
cx.export_function("unstash_global_object", js::workers::unstash_global_object)?;
cx.export_function("reject_after", js::workers::reject_after)?;
cx.export_function("box_channels", js::workers::box_channels)?;

// Futures
cx.export_function("lazy_async_add", js::futures::lazy_async_add)?;
Expand Down