addons: allow multiple requires of the same addon

[frutiger]

Replace the global state tracking a single pending addon that is to be
loaded with a map from addon name to node_module*. This allows
multiple, independent node::Environments to load the same addon.

Add a test to validate this new behavior by clearing the require cache
and reloading the addon.

NOTE: This will break any user that is not following the documentation
and requiring an addon via a different name than that passed as the
first argument to NODE_MODULE(...).

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[frutiger]

This commit also fixes a long-standing FIXME left by @bnoordhuis over four years ago when they added multi-context support.

[frutiger]

@danbev @Trott @addaleax may also be interested.

[Trott]

@nodejs/collaborators This has been open for 3 days but hasn't received any reviews/comments.

[mcollina]

NOTE: This will break any user that is not following the documentation
and requiring an addon via a different name than that passed as the
first argument to NODE_MODULE(...).

I flagged it as semver-major for any potential breakage.

[mcollina]

LGTM with a nit.

[mcollina]

Can you put the full error message here?

[frutiger]

@mcollina I updated the test.

[mhdawson]

I think this needs a test for n-api modules as well as the functionality for both n-api and n-api modules needs to be on par.

[frutiger]

@mhdawson I added the NAPI test, and validated that this change makes it pass.

[frutiger]

I have currently only tested the changes on macOS. Is there a way to trigger the CI system to build these changes? I was not able to find anything on https://ci.nodejs.org/job/node-test-pull-request/ that would let me trigger a job (nor any documentation).

[gabrielschulhof]

I don't get it. It's already possible to require() an addon multiply since 3828fc6. In fact, the library loader maintains precisely such a filename-to-library-handle map so that it can determine whether a library has already been loaded. We need not duplicate that functionality inside Node.js.

[gabrielschulhof]

I have created #19875 to test the case where a module is loaded twice.

[frutiger]

@gabrielschulhof the fix from @bnoordhuis in 3828fc6 works very well if you use the well-known symbol approach to define an add-on, but does not work so well if you use to other well-documented method of using the macro to define an add-on.

My PR fixes the latter case. The former case is not a problem since the change in 3828fc6.

[frutiger]

This is especially problematic since the Electron project introduced support for the affinity tag. This allows multiple renderers to run sharing the same process, v8::Isolate an libuv event loop, which in turn, involves running multiple node::Environments sharing all of the above.

If each of these environments are truly independent, they should be allowed to load a Node add-on built using either the standard macro or using the well-known symbol.

[gabrielschulhof]

@frutiger OK, right, if the well-known symbol is absent, you have to maintain a list of already-loaded modules keyed by their requested path. Thanks for clarifying!

[gabrielschulhof]

I think we need a stronger key because this also breaks those projects which use two different versions of the same module. So, like,

project
  |
  +-dep1
  |   |
  |   +-native-addon1(@2.2.0)
  |
  +-native-addon1(@1.2.0)

The distinction between the two modules with identical nm_modname is the path from which they are loaded. So we should save the absolute path of the module we are about to load to a global variable declared next to modpending right before calling dlib.Open(), and then use that path as the key here.

So, like

 static node_module* modpending;
+std::string modpending_filename;

and then

mods[modpending_filename.c_str()] = mp;

in node_module_register(), and

+modpending_filename = std::string(*filename);
 bool is_opened = dlib.Open();

in DLOpen().
In general, I have reservations about attempting to duplicate what the library loader is doing, because that code has very likely been refined to a very high degree over the course of the decades.

[frutiger]

You are absolutely right, the basename is not unambiguous enough.

I can change it as you suggest - in fact we don't even need to save the pending filename, we can just he the pending node_module * as we do today and use its file name as the key in the std::unordered_map.

[gabrielschulhof]

@frutiger we must also ensure that two different file names which refer to the same file in reality are treated correctly. I'm not sure to what extent the JS side of require() takes care of resolving files in this way.

[gabrielschulhof]

@frutiger it might be worth also taking a look at the JS side, and maybe adding some more resolution steps to the portion of the code path following the decision to load a native module.

[gabrielschulhof]

@frutiger I guess we should at least follow symlinks.

[frutiger]

I'm not actually sure just following symlinks is enough, nor do I think we would be able to do a good job of maintaining compatibility of this logic with the loader logic in JS. Perhaps we could hash the contents of the file itself?

[gabrielschulhof]

Ideally, the JS loader logic would cause the resulting call to dlopen() to always return a new module, and run the constructors. However, this means that we will have reproduced dlopen()'s decision as to whether to load a library or increment the refcount of an existing one. I'm pretty sure we haven't done that.

The decision made by dlopen() as to whether to load a new library, thereby running its constructors, or incrementing the reference count of an existing library, thereby not running its constructors, is what we're trying to replicate here. The code for that decision is very old, very well-vetted, and may even be platform-dependent. This is why I said I was reluctant to attempt to reproduce that code.

In an ideal world we'd have dlreopen() or the RTLD_REOPEN flag which would do exactly what we need. But alas.

[gabrielschulhof]

I guess @bnoordhuis' modification whereby it falls back to looking for a well-known symbol is a way to kick-start the addon if it fails to register. But, like you said, this won't work for existing modules. sigh

[mhdawson]

triggering CI as requested above to get broader platform coverage: https://ci.nodejs.org/job/node-test-pull-request/14157/

[bnoordhuis]

I appreciate you're taking this on but this approach is not safe. Many add-ons do global initialization in their init hook. Doing that twice will result in... interesting... effects.

Red X to counter @mcollina's LGTM. This can't land as-is.

dismissed based on @bnoordhuis comment.

[frutiger]

For our use case, I can take the well-known symbol approach, so I appreciate all the feedback and I don't actually need the change proposed here.

I am curious though @bnoordhuis: if an add-on does global initialization on load, it clearly cannot work across multiple contexts, and so is it worth attempting to support them for the multi-use case?

[gabrielschulhof]

@frutiger you can attach state during Init to the exports object that you return, and you can pass that state to every function that you expose via napi_create_function's void* data parameter. IOW, you can entirely avoid storing state globally.

You could also create a global map keyed on the napi_env env to store per-env state and then access the map in a thread-safe manner.

[gabrielschulhof]

... so any and all addons that will want/claim to support multi-context will have to be modified to store their state in such a fashion.

[frutiger]

We will be personally changing our add-ons to use the well-known symbol instead of using a load-time registration function. Abandoning this PR as a result.