src: fix race on modpending by migrating it to a thread local #21611

rpetrich · 2018-06-30T18:14:51Z

Fixes a rare race condition on modpending when two native modules are loaded simultaneously on different threads by migrating modpending to be a thread local.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows [commit guidelines]

addaleax

Thanks for the PR!

Just fyi, there are some alternative approaches that address this problem more completely – #20759 goes a bit into this direction (or at least touches a lot of this code), and I wanted to fix up addaleax/node@541ec98 to work as a “proper” solution to this problem.

In particular, Node shouldn’t load addons from Workers that don’t explicitly state that they support being loaded in such an environment – Many addons create some kind of libuv handles, and don’t install proper cleanup hooks for when the Worker shuts down that close those handles

If you want to help with that, that would be appreciated a lot 💙

addaleax · 2018-06-30T18:20:39Z

src/node.cc

@@ -1144,7 +1143,7 @@ extern "C" void node_module_register(void* m) {
    mp->nm_link = modlist_linked;
    modlist_linked = mp;
  } else {
-    modpending = mp;
+    Environment::GetThreadLocalEnv()->modpending = mp;


Almost any use of Environment::GetThreadLocalEnv() is going to be buggy, because Node.js generally seeks to support embedders who create multiple Environments per thread :(

@addaleax Thanks, I was unaware of that. I've migrated to using a thread local instead, which should properly support multiple Environments per thread.

Fixes a rare race condition on modpending when two native modules are loaded simultaneously on different threads by migrating modpending to be a member of Environment.

gabrielschulhof · 2018-07-11T02:21:04Z

@addaleax modules which rely on mp are not multiple init anyway, because relying on mp is equivalent to relying on library constructors, which are once-per-process-per-module.

Assuming the node environment on such a thread is correctly set up to be indistinguishable from a single-threaded node environment such addons will work correctly because the one thread where they do load is the only thread where they need to work and they will not subsequently load because the library constructors will not fire again.

In contrast, when using a special symbol a module must support multiple init and module-instance-local storage, because such a symbol will be found every time the module is loaded, no matter what thread it's on and no matter that it's multiple times on a single thread.

So, I think landing this PR will help us, and will not cause any more breakage than the current inability to load a module more than once already causes.

gabrielschulhof · 2018-07-11T22:22:13Z

src/node.cc

@@ -185,7 +185,8 @@ static int v8_thread_pool_size = v8_default_thread_pool_size;
 static bool prof_process = false;
 static bool v8_is_profiling = false;
 static bool node_is_initialized = false;
-static node_module* modpending;
+static uv_once_t init_once = UV_ONCE_INIT;


The name of this variable should indicate that it refers to thread_local_modpending – so, like, maybe init_modpending_once?

gabrielschulhof · 2018-07-11T22:23:35Z

src/node.cc

@@ -1258,6 +1259,10 @@ inline napi_addon_register_func GetNapiInitializerCallback(DLib* dlib) {
      reinterpret_cast<napi_addon_register_func>(dlib->GetSymbolAddress(name));
 }

+void InitDLOpenOnce() {


This should be InitModpendingOnce, not InitDLOpenOnce.

gabrielschulhof · 2018-07-19T14:08:43Z

I was able to cause node --experimental-worker to abort with the following message:

node[25666]: ../src/node.cc:1783:void node::DLOpen(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(modpending) == nullptr' failed.
 1: 0x89a0c0 node::Abort() [node]
 2: 0x89a1a5  [node]
 3: 0x89b38a  [node]
 4: 0xb14459  [node]
 5: 0xb14fc9 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 6: 0x685b99041bd

by running the following code:

/* index.js */
const assert = require('assert');
const {
  Worker, isMainThread, parentPort
} = require('worker_threads');

if (isMainThread) {
  let robinHood;
  (new Worker(__filename)).on('message', (data) => {
    if (data === 'ready') {
      robinHood = JSON.stringify(require('./build/Release/robin_hood'));
    } else {
      console.log(data + '\nvs.\n' + robinHood);
      assert.notStrictEqual(data, robinHood);
    }
  });
} else {
  parentPort.postMessage('ready');
  parentPort.postMessage(JSON.stringify(require('./build/Release/friar_tuck')));
}

where

/* robin_hood.c */
#include <assert.h>
#include <node_api.h>

static napi_value Init(napi_env env, napi_value exports) {
  napi_status status;
  napi_value result;

  status = napi_create_uint32(env, 42, &result);
  assert(status == napi_ok);

  status = napi_set_named_property(env, exports, "answer", result);
  assert(status == napi_ok);

  return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

and

/* friar_tuck.c */
#include <assert.h>
#include <node_api.h>

static napi_value Init(napi_env env, napi_value exports) {
  napi_status status;
  napi_value result;

  status = napi_create_string_utf8(env, "good", NAPI_AUTO_LENGTH, &result);
  assert(status == napi_ok);

  status = napi_set_named_property(env, exports, "question", result);
  assert(status == napi_ok);

  return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

in a shell loop with

while node --experimental-worker ./index.js; do echo -n ''; done

It's not deterministic though so it doesn't serve as a good test.

gabrielschulhof · 2018-07-19T14:23:31Z

@addaleax I believe that using NODE_MODULE_INIT() or NAPI_MODULE_INIT() instead of NODE_MODULE() and NAPI_MODULE() constitutes opt-in, especially given the NODE_MODULE_INIT() documentation we recently added.

In conclusion, using NODE_MODULE() or NAPI_MODULE() will result in a single-load module, unless the symbol given as the nm_register_func is the well-known symbol (which is most often not the case in the wild), and using NODE_MODULE_INIT() or NAPI_MODULE_INIT() will result in a contex-aware, multiple-load module. With this change, both scenarios will work correctly AFAICT.

mhdawson · 2018-07-19T18:22:15Z

@gabrielf that all makes sense to me. I guess we just need to get this PR updated based on your comments and the it looks good to me.

gabrielschulhof · 2018-07-19T18:29:09Z

@rpetrich do you think you might be able to find the time to address my comments so we can move the PR forward?

gabrielschulhof · 2018-07-27T21:13:13Z

@mhdawson, @addaleax I fixed the naming. Could you please have another look?

rpetrich · 2018-07-27T23:33:10Z

@gabrielschulhof Thanks for pushing to the PR. I'd missed the earlier comments about naming.

gabrielschulhof · 2018-07-28T00:02:53Z

@rpetrich happy to help!

gabrielschulhof · 2018-08-08T02:48:35Z

@mhdawson could you PTAL?

gabrielschulhof · 2018-08-11T20:32:31Z

@nodejs/addon-api could someone else please also take a look?

gabrielschulhof · 2018-08-13T13:44:53Z

@nodejs/collaborators could someone please have a look?

Trott · 2018-08-14T18:57:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16448/

gabrielschulhof · 2018-08-16T13:16:25Z

CI resumed as https://ci.nodejs.org/job/node-test-pull-request/16483/

gabrielschulhof · 2018-08-16T13:23:51Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/16484/

gabrielschulhof · 2018-08-16T14:07:53Z

FreeBSD rebuild: https://ci.nodejs.org/job/node-test-commit-freebsd/19726/

Trott · 2018-08-16T22:21:52Z

I think FreeBSD is back to green. Let's try a Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16501/

gabrielschulhof · 2018-08-17T00:00:39Z

Landed in 3814534.

Fixes a rare race condition on modpending when two native modules are loaded simultaneously on different threads by storing it thread- locally. PR-URL: #21611 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 30, 2018

addaleax reviewed Jun 30, 2018

View reviewed changes

rpetrich force-pushed the fix-modpending-race-master branch from 18b1cde to 5c3e676 Compare June 30, 2018 19:38

src: fix race on modpending by migrating it to Environment

7f2c8bf

Fixes a rare race condition on modpending when two native modules are loaded simultaneously on different threads by migrating modpending to be a member of Environment.

rpetrich force-pushed the fix-modpending-race-master branch from 5c3e676 to 7f2c8bf Compare June 30, 2018 19:38

rpetrich changed the title ~~src: fix race on modpending by migrating it to Environment~~ src: fix race on modpending by migrating it to a thread local Jun 30, 2018

gabrielschulhof reviewed Jul 11, 2018

View reviewed changes

devsnek mentioned this pull request Jul 12, 2018

worker threads throws exception while loading npm lib #21783

Closed

src: squash! fix naming

da4632a

gabrielschulhof approved these changes Aug 11, 2018

View reviewed changes

BridgeAR requested review from bnoordhuis, jasnell, addaleax, TimothyGu, devsnek and mhdawson August 13, 2018 13:54

devsnek removed their request for review August 13, 2018 15:25

gabrielschulhof mentioned this pull request Aug 14, 2018

doc: require two approvals to land changes #22255

Closed

3 tasks

joyeecheung mentioned this pull request Aug 15, 2018

CI failures: 20180815 nodejs/reliability#13

Closed

TimothyGu removed their request for review August 16, 2018 00:39

apapirovski approved these changes Aug 16, 2018

View reviewed changes

gabrielschulhof closed this Aug 17, 2018

joyeecheung mentioned this pull request Aug 17, 2018

CI Failures: 20180817 nodejs/reliability#14

Closed

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix race on modpending by migrating it to a thread local #21611

src: fix race on modpending by migrating it to a thread local #21611

rpetrich commented Jun 30, 2018 •

edited

Loading

addaleax left a comment

addaleax Jun 30, 2018

rpetrich Jun 30, 2018

gabrielschulhof commented Jul 11, 2018

gabrielschulhof Jul 11, 2018

gabrielschulhof Jul 11, 2018

gabrielschulhof commented Jul 19, 2018 •

edited

Loading

gabrielschulhof commented Jul 19, 2018

mhdawson commented Jul 19, 2018

gabrielschulhof commented Jul 19, 2018

gabrielschulhof commented Jul 27, 2018

rpetrich commented Jul 27, 2018

gabrielschulhof commented Jul 28, 2018

gabrielschulhof commented Aug 8, 2018

gabrielschulhof commented Aug 11, 2018

gabrielschulhof commented Aug 13, 2018

Trott commented Aug 14, 2018

gabrielschulhof commented Aug 16, 2018

gabrielschulhof commented Aug 16, 2018

gabrielschulhof commented Aug 16, 2018

Trott commented Aug 16, 2018

gabrielschulhof commented Aug 17, 2018

src: fix race on modpending by migrating it to a thread local #21611

src: fix race on modpending by migrating it to a thread local #21611

Conversation

rpetrich commented Jun 30, 2018 • edited Loading

Checklist

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jun 30, 2018

Choose a reason for hiding this comment

rpetrich Jun 30, 2018

Choose a reason for hiding this comment

gabrielschulhof commented Jul 11, 2018

gabrielschulhof Jul 11, 2018

Choose a reason for hiding this comment

gabrielschulhof Jul 11, 2018

Choose a reason for hiding this comment

gabrielschulhof commented Jul 19, 2018 • edited Loading

gabrielschulhof commented Jul 19, 2018

mhdawson commented Jul 19, 2018

gabrielschulhof commented Jul 19, 2018

gabrielschulhof commented Jul 27, 2018

rpetrich commented Jul 27, 2018

gabrielschulhof commented Jul 28, 2018

gabrielschulhof commented Aug 8, 2018

gabrielschulhof commented Aug 11, 2018

gabrielschulhof commented Aug 13, 2018

Trott commented Aug 14, 2018

gabrielschulhof commented Aug 16, 2018

gabrielschulhof commented Aug 16, 2018

gabrielschulhof commented Aug 16, 2018

Trott commented Aug 16, 2018

gabrielschulhof commented Aug 17, 2018

rpetrich commented Jun 30, 2018 •

edited

Loading

gabrielschulhof commented Jul 19, 2018 •

edited

Loading