Skip to content

Commit efc57aa

Browse files
committed
worker: improve integration with native addons
Native addons are now unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. Thanks go to Alexey Orlenko, Olivia Hugger and Stephen Belanger for reviewing the original version of this change. Refs: ayojs/ayo#118
1 parent a9bb653 commit efc57aa

File tree

6 files changed

+255
-76
lines changed

6 files changed

+255
-76
lines changed

src/node.cc

Lines changed: 168 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,17 @@ node_module* get_linked_module(const char* name) {
11351135
return FindModule(modlist_linked, name, NM_F_LINKED);
11361136
}
11371137

1138-
class DLib {
1138+
namespace {
1139+
1140+
Mutex dlib_mutex;
1141+
1142+
class DLib;
1143+
1144+
std::unordered_map<std::string, std::shared_ptr<DLib>> dlopen_cache;
1145+
std::unordered_map<decltype(uv_lib_t().handle), std::shared_ptr<DLib>>
1146+
handle_to_dlib;
1147+
1148+
class DLib : public std::enable_shared_from_this<DLib> {
11391149
public:
11401150
#ifdef __POSIX__
11411151
static const int kDefaultFlags = RTLD_LAZY;
@@ -1146,17 +1156,27 @@ class DLib {
11461156
inline DLib(const char* filename, int flags)
11471157
: filename_(filename), flags_(flags), handle_(nullptr) {}
11481158

1159+
inline DLib() {}
1160+
inline ~DLib() {
1161+
Close();
1162+
}
1163+
11491164
inline bool Open();
11501165
inline void Close();
11511166
inline void* GetSymbolAddress(const char* name);
1167+
inline void AddEnvironment(Environment* env);
11521168

1153-
const std::string filename_;
1154-
const int flags_;
1169+
std::string filename_;
1170+
int flags_;
11551171
std::string errmsg_;
1156-
void* handle_;
1172+
void* handle_ = nullptr;
1173+
std::unordered_set<Environment*> users_;
1174+
node_module* own_info = nullptr;
1175+
11571176
#ifndef __POSIX__
11581177
uv_lib_t lib_;
11591178
#endif
1179+
11601180
private:
11611181
DISALLOW_COPY_AND_ASSIGN(DLib);
11621182
};
@@ -1225,18 +1245,49 @@ void InitModpendingOnce() {
12251245
CHECK_EQ(0, uv_key_create(&thread_local_modpending));
12261246
}
12271247

1248+
void DLib::AddEnvironment(Environment* env) {
1249+
if (users_.count(env) > 0) return;
1250+
users_.insert(env);
1251+
if (env->is_main_thread()) return;
1252+
struct cleanup_hook_data {
1253+
std::shared_ptr<DLib> info;
1254+
Environment* env;
1255+
};
1256+
env->AddCleanupHook([](void* arg) {
1257+
Mutex::ScopedLock lock(dlib_mutex);
1258+
std::unique_ptr<cleanup_hook_data> cbdata(
1259+
static_cast<cleanup_hook_data*>(arg));
1260+
std::shared_ptr<DLib> info = cbdata->info;
1261+
info->users_.erase(cbdata->env);
1262+
1263+
if (info->users_.empty()) {
1264+
// No more Environments left using this DLib -> remove filename from
1265+
// caches.
1266+
std::vector<std::string> filenames;
1267+
1268+
for (const auto& entry : dlopen_cache) {
1269+
if (entry.second == info)
1270+
filenames.push_back(entry.first);
1271+
}
1272+
for (const std::string& filename : filenames)
1273+
dlopen_cache.erase(filename);
1274+
1275+
handle_to_dlib.erase(info->handle_);
1276+
}
1277+
}, static_cast<void*>(new cleanup_hook_data { shared_from_this(), env }));
1278+
}
1279+
1280+
} // anonymous namespace
1281+
12281282
// DLOpen is process.dlopen(module, filename, flags).
12291283
// Used to load 'module.node' dynamically shared objects.
1230-
//
1231-
// FIXME(bnoordhuis) Not multi-context ready. TBD how to resolve the conflict
1232-
// when two contexts try to load the same shared object. Maybe have a shadow
1233-
// cache that's a plain C list or hash table that's shared across contexts?
12341284
static void DLOpen(const FunctionCallbackInfo<Value>& args) {
12351285
Environment* env = Environment::GetCurrent(args);
1236-
auto context = env->context();
1237-
12381286
uv_once(&init_modpending_once, InitModpendingOnce);
1239-
CHECK_NULL(uv_key_get(&thread_local_modpending));
1287+
1288+
Local<Context> context = env->context();
1289+
node_module* mp;
1290+
std::shared_ptr<DLib> dlib;
12401291

12411292
if (args.Length() < 2) {
12421293
env->ThrowError("process.dlopen needs at least 2 arguments.");
@@ -1257,83 +1308,128 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
12571308
return; // Exception pending.
12581309
}
12591310

1260-
node::Utf8Value filename(env->isolate(), args[1]); // Cast
1261-
DLib dlib(*filename, flags);
1262-
bool is_opened = dlib.Open();
1311+
// Use a do { ... } while(false) loop to be able to break out early
1312+
// if a cached entry was found.
1313+
do {
1314+
CHECK_NULL(uv_key_get(&thread_local_modpending));
1315+
Mutex::ScopedLock lock(dlib_mutex);
1316+
1317+
if (args.Length() < 2) {
1318+
env->ThrowError("process.dlopen needs at least 2 arguments.");
1319+
return;
1320+
}
1321+
1322+
int32_t flags = DLib::kDefaultFlags;
1323+
if (args.Length() > 2 && !args[2]->Int32Value(context).To(&flags)) {
1324+
return env->ThrowTypeError("flag argument must be an integer.");
1325+
}
1326+
1327+
node::Utf8Value filename(env->isolate(), args[1]); // Cast
1328+
auto it = dlopen_cache.find(*filename);
1329+
1330+
if (it != dlopen_cache.end()) {
1331+
dlib = it->second;
1332+
mp = dlib->own_info;
1333+
dlib->AddEnvironment(env);
1334+
break;
1335+
}
1336+
1337+
dlib = std::make_shared<DLib>();
1338+
dlib->filename_ = *filename;
1339+
dlib->flags_ = flags;
1340+
bool is_opened = dlib->Open();
12631341

1264-
// Objects containing v14 or later modules will have registered themselves
1265-
// on the pending list. Activate all of them now. At present, only one
1266-
// module per object is supported.
1267-
node_module* const mp = static_cast<node_module*>(
1268-
uv_key_get(&thread_local_modpending));
1269-
uv_key_set(&thread_local_modpending, nullptr);
1342+
if (is_opened && handle_to_dlib.count(dlib->handle_) > 0) {
1343+
dlib = handle_to_dlib[dlib->handle_];
1344+
mp = dlib->own_info;
1345+
dlib->AddEnvironment(env);
1346+
break;
1347+
}
12701348

1271-
if (!is_opened) {
1272-
Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
1273-
dlib.Close();
1349+
// Objects containing v14 or later modules will have registered themselves
1350+
// on the pending list. Activate all of them now. At present, only one
1351+
// module per object is supported.
1352+
mp = static_cast<node_module*>(uv_key_get(&thread_local_modpending));
1353+
uv_key_set(&thread_local_modpending, nullptr);
1354+
1355+
if (!is_opened) {
1356+
Local<String> errmsg =
1357+
OneByteString(env->isolate(), dlib->errmsg_.c_str());
12741358
#ifdef _WIN32
1275-
// Windows needs to add the filename into the error message
1276-
errmsg = String::Concat(
1277-
env->isolate(), errmsg, args[1]->ToString(context).ToLocalChecked());
1359+
// Windows needs to add the filename into the error message
1360+
errmsg = String::Concat(env->isolate(),
1361+
errmsg,
1362+
args[1]->ToString(context).ToLocalChecked());
12781363
#endif // _WIN32
1279-
env->isolate()->ThrowException(Exception::Error(errmsg));
1280-
return;
1281-
}
1364+
env->isolate()->ThrowException(Exception::Error(errmsg));
1365+
return;
1366+
}
12821367

1283-
if (mp == nullptr) {
1284-
if (auto callback = GetInitializerCallback(&dlib)) {
1285-
callback(exports, module, context);
1286-
} else if (auto napi_callback = GetNapiInitializerCallback(&dlib)) {
1287-
napi_module_register_by_symbol(exports, module, context, napi_callback);
1288-
} else {
1289-
dlib.Close();
1290-
env->ThrowError("Module did not self-register.");
1368+
if (mp == nullptr) {
1369+
if (auto callback = GetInitializerCallback(dlib.get())) {
1370+
callback(exports, module, context);
1371+
} else if (auto napi_callback = GetNapiInitializerCallback(dlib.get())) {
1372+
napi_module_register_by_symbol(exports, module, context, napi_callback);
1373+
} else {
1374+
dlib->Close();
1375+
env->ThrowError("Module did not self-register.");
1376+
}
1377+
return;
12911378
}
1292-
return;
1293-
}
12941379

1295-
// -1 is used for N-API modules
1296-
if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
1297-
// Even if the module did self-register, it may have done so with the wrong
1298-
// version. We must only give up after having checked to see if it has an
1299-
// appropriate initializer callback.
1300-
if (auto callback = GetInitializerCallback(&dlib)) {
1301-
callback(exports, module, context);
1380+
// -1 is used for N-API modules
1381+
if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
1382+
// Even if the module did self-register, it may have done so with the
1383+
// wrong version. We must only give up after having checked to see if it
1384+
// has an appropriate initializer callback.
1385+
if (auto callback = GetInitializerCallback(dlib.get())) {
1386+
callback(exports, module, context);
1387+
return;
1388+
}
1389+
char errmsg[1024];
1390+
snprintf(errmsg,
1391+
sizeof(errmsg),
1392+
"The module '%s'"
1393+
"\nwas compiled against a different Node.js version using"
1394+
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
1395+
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
1396+
"re-installing\nthe module (for instance, using `npm rebuild` "
1397+
"or `npm install`).",
1398+
*filename, mp->nm_version, NODE_MODULE_VERSION);
1399+
1400+
// NOTE: `mp` is allocated inside of the shared library's memory,
1401+
// calling `dlclose` will deallocate it
1402+
env->ThrowError(errmsg);
13021403
return;
13031404
}
1304-
char errmsg[1024];
1305-
snprintf(errmsg,
1306-
sizeof(errmsg),
1307-
"The module '%s'"
1308-
"\nwas compiled against a different Node.js version using"
1309-
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
1310-
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
1311-
"re-installing\nthe module (for instance, using `npm rebuild` "
1312-
"or `npm install`).",
1313-
*filename, mp->nm_version, NODE_MODULE_VERSION);
1314-
1315-
// NOTE: `mp` is allocated inside of the shared library's memory, calling
1316-
// `dlclose` will deallocate it
1317-
dlib.Close();
1318-
env->ThrowError(errmsg);
1319-
return;
1320-
}
1321-
if (mp->nm_flags & NM_F_BUILTIN) {
1322-
dlib.Close();
1323-
env->ThrowError("Built-in module self-registered.");
1324-
return;
1325-
}
13261405

1327-
mp->nm_dso_handle = dlib.handle_;
1328-
mp->nm_link = modlist_addon;
1329-
modlist_addon = mp;
1406+
if (mp->nm_flags & NM_F_BUILTIN) {
1407+
env->ThrowError("Built-in module self-registered.");
1408+
return;
1409+
}
1410+
1411+
if (mp->nm_context_register_func == nullptr &&
1412+
mp->nm_register_func == nullptr) {
1413+
env->ThrowError("Module has no declared entry point.");
1414+
return;
1415+
}
1416+
1417+
dlib->own_info = mp;
1418+
handle_to_dlib[dlib->handle_] = dlib;
1419+
dlopen_cache[*filename] = dlib;
1420+
1421+
dlib->AddEnvironment(env);
1422+
1423+
mp->nm_dso_handle = dlib->handle_;
1424+
mp->nm_link = modlist_addon;
1425+
modlist_addon = mp;
1426+
} while (false);
13301427

13311428
if (mp->nm_context_register_func != nullptr) {
13321429
mp->nm_context_register_func(exports, module, context, mp->nm_priv);
13331430
} else if (mp->nm_register_func != nullptr) {
13341431
mp->nm_register_func(exports, module, mp->nm_priv);
13351432
} else {
1336-
dlib.Close();
13371433
env->ThrowError("Module has no declared entry point.");
13381434
return;
13391435
}

test/addons/dlopen-ping-pong/test.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,5 @@ process.dlopen(module, bindingPath,
1414
module.exports.load(`${path.dirname(bindingPath)}/ping.so`);
1515
assert.strictEqual(module.exports.ping(), 'pong');
1616

17-
// Check that after the addon is loaded with
18-
// process.dlopen() a require() call fails.
19-
const re = /^Error: Module did not self-register\.$/;
20-
assert.throws(() => require(`./build/${common.buildType}/binding`), re);
17+
// This second `require()` call should not throw an error.
18+
require(`./build/${common.buildType}/binding`);
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
const common = require('../../common');
4+
const assert = require('assert');
5+
const path = require('path');
6+
const { Worker } = require('worker_threads');
7+
const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
8+
9+
const w = new Worker(`
10+
require('worker_threads').parentPort.postMessage(
11+
require(${JSON.stringify(binding)}).hello());`, { eval: true });
12+
w.on('message', common.mustCall((message) => {
13+
assert.strictEqual(message, 'world');
14+
}));

test/addons/worker-addon/binding.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#include <assert.h>
2+
#include <stdlib.h>
3+
#include <stdio.h>
4+
#include <node.h>
5+
#include <v8.h>
6+
7+
using v8::Context;
8+
using v8::HandleScope;
9+
using v8::Isolate;
10+
using v8::Local;
11+
using v8::Object;
12+
using v8::Value;
13+
14+
size_t count = 0;
15+
16+
struct statically_allocated {
17+
statically_allocated() {
18+
assert(count == 0);
19+
printf("ctor ");
20+
}
21+
~statically_allocated() {
22+
assert(count == 0);
23+
printf("dtor");
24+
}
25+
} var;
26+
27+
void Dummy(void*) {
28+
assert(0);
29+
}
30+
31+
void Cleanup(void* str) {
32+
printf("%s ", static_cast<const char*>(str));
33+
}
34+
35+
void Initialize(Local<Object> exports,
36+
Local<Value> module,
37+
Local<Context> context) {
38+
node::AddEnvironmentCleanupHook(
39+
context->GetIsolate(), Cleanup,
40+
const_cast<void*>(static_cast<const void*>("cleanup")));
41+
node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
42+
node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
43+
}
44+
45+
NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize)

test/addons/worker-addon/binding.gyp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'binding',
5+
'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
6+
'sources': [ 'binding.cc' ]
7+
}
8+
]
9+
}

test/addons/worker-addon/test.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
const common = require('../../common');
4+
const assert = require('assert');
5+
const child_process = require('child_process');
6+
const path = require('path');
7+
const { Worker } = require('worker_threads');
8+
const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
9+
10+
if (process.argv[2] === 'child') {
11+
new Worker(`require(${JSON.stringify(binding)});`, { eval: true });
12+
} else {
13+
const proc = child_process.spawnSync(process.execPath, [__filename, 'child']);
14+
assert.strictEqual(proc.stderr.toString(), '');
15+
assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor');
16+
assert.strictEqual(proc.status, 0);
17+
}

0 commit comments

Comments
 (0)