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(napi): Fix worker threads importing already-loaded NAPI addon #25245

Merged
merged 3 commits into from
Aug 28, 2024
Merged
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
23 changes: 23 additions & 0 deletions ext/napi/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ use core::ptr::NonNull;
use deno_core::error::type_error;
use deno_core::error::AnyError;
use deno_core::op2;
use deno_core::parking_lot::RwLock;
use deno_core::url::Url;
use deno_core::ExternalOpsTracker;
use deno_core::OpState;
use deno_core::V8CrossThreadTaskSpawner;
use std::cell::RefCell;
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
use std::rc::Rc;
Expand Down Expand Up @@ -493,6 +495,15 @@ impl NapiPermissions for deno_permissions::PermissionsContainer {
}
}

unsafe impl Sync for NapiModuleHandle {}
unsafe impl Send for NapiModuleHandle {}
#[derive(Clone, Copy)]
struct NapiModuleHandle(*const NapiModule);

static NAPI_LOADED_MODULES: std::sync::LazyLock<
RwLock<HashMap<String, NapiModuleHandle>>,
> = std::sync::LazyLock::new(|| RwLock::new(HashMap::new()));

#[op2(reentrant)]
fn op_napi_open<NP, 'scope>(
scope: &mut v8::HandleScope<'scope>,
Expand Down Expand Up @@ -575,11 +586,23 @@ where
let exports = v8::Object::new(scope);

let maybe_exports = if let Some(module_to_register) = maybe_module {
NAPI_LOADED_MODULES
.write()
.insert(path, NapiModuleHandle(module_to_register));
// SAFETY: napi_register_module guarantees that `module_to_register` is valid.
let nm = unsafe { &*module_to_register };
assert_eq!(nm.nm_version, 1);
// SAFETY: we are going blind, calling the register function on the other side.
unsafe { (nm.nm_register_func)(env_ptr, exports.into()) }
} else if let Some(module_to_register) =
{ NAPI_LOADED_MODULES.read().get(&path).copied() }
{
// SAFETY: this originated from `napi_register_module`, so the
// pointer should still be valid.
let nm = unsafe { &*module_to_register.0 };
assert_eq!(nm.nm_version, 1);
// SAFETY: we are going blind, calling the register function on the other side.
unsafe { (nm.nm_register_func)(env_ptr, exports.into()) }
} else if let Ok(init) = unsafe {
library.get::<napi_register_module_v1>(b"napi_register_module_v1")
} {
Expand Down
35 changes: 35 additions & 0 deletions tests/napi/init_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { Buffer } from "node:buffer";
import { assert, libSuffix } from "./common.js";
import { Worker } from "node:worker_threads";

const ops = Deno[Deno.internal].core.ops;

Expand All @@ -13,3 +14,37 @@ Deno.test("ctr initialization (napi_module_register)", {
assert(obj != null);
assert(typeof obj === "object");
});

Deno.test("ctr initialization by multiple threads (napi_module_register)", {
ignore: Deno.build.os == "windows",
}, async function () {
const path = new URL(`./module.${libSuffix}`, import.meta.url).pathname;
const obj = ops.op_napi_open(path, {}, Buffer, reportError);
const common = import.meta.resolve("./common.js");
assert(obj != null);
assert(typeof obj === "object");

const worker = new Worker(
`
import { Buffer } from "node:buffer";
import { parentPort } from "node:worker_threads";
import { assert } from "${common}";

const ops = Deno[Deno.internal].core.ops;
const obj = ops.op_napi_open("${path}", {}, Buffer, reportError);
assert(obj != null);
assert(typeof obj === "object");
parentPort.postMessage("ok");
`,
{
eval: true,
},
);

const p = Promise.withResolvers();
worker.on("message", (_m) => {
p.resolve();
});

await p.promise;
});