src: add addon entry point symbol cache

[gabrielschulhof]

Determine whether the dlopen() of a certain addon is the first load of
said addon, or if the addon had previously been loaded. This is
accomplished by storing the addon's entry point in a set. If, upon a
subsequent load, after determining the addon's entry point, it is found
to already be present in the cache, then the handle returned by the
subsequent dlopen() is closed.

Checklist

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

[addaleax]

It’s hard to tell the actual behavioural differences here – as far as I can tell:

• We call .Close() if we have already loaded the addon. I think at least on POSIX systems that won’t make much of a difference?
• The bailout when an N-API addon has a NULL entry point. That seems like a good idea, but I’m not sure how it depends on the rest?

[addaleax]

Can this rather be an inline function?

[gabrielschulhof]

I'm not sure how to capture all the different ways we initialize an addon in a single inline function.

We have

• callback(exports, module, context) for well-known native addon symbols
• napi_module_register_by_symbol(exports, module, context, napi_callback) for N-API
• mp->nm_context_register_func(exports, module, context, mp->nm_priv) for self-registering context-aware addons
• mp->nm_register_func(exports, module, mp->nm_priv) for self-registering non-context-aware addons

[gabrielschulhof]

Making some headway with lambdas. Lessee ...

[addaleax]

Style nit: Indentation is off here

[addaleax]

Oh, also: Maybe take a look at addaleax/node@1d8b74d. It’s WIP and not going to be PR’ed soon, but that’s the direction into which I’d like to move the addon loading mechanism :)

[bnoordhuis]

Can be DRY'd to this:

if (!addon_entry_points.insert(key_value).second) dlib.Close();

Apropos the name key_value, that to me suggests it's a std::pair when it's not.

[gabrielschulhof]

@addaleax I've left a comment on your commit: addaleax@1d8b74d#commitcomment-28999355. tl;dr: Awesome work, but please use the module entry point rather than the module file name as the key.

[gabrielschulhof]

@addaleax I've replaced the macro with an inline function.

@bnoordhuis I've switched to entry_point.

[gabrielschulhof]

@addaleax I've separated out the N-API null-checking into its own PR.

[fs-eire]

does this change also allows an addon defined by NODE_MODULE() being loaded in multiple context?

[gabrielschulhof]

@fs-eire the ability to re-initialize an addon, possibly with a different context, has already been available since 3828fc6.

This change merely makes sure that dlopen() handles do not leak as you load a given module multiple times.

So, with this change, for any given module, if dlopen() is called more than once, then the second and subsequent times dlclose() will be called immediately afterwards, thereby ensuring that only the first dlopen() handle remains open.

[gabrielschulhof]

Running CI https://ci.nodejs.org/job/node-test-pull-request/14966/ to see if there are any platform-specific issues.

[fs-eire]

@gabrielschulhof
my understanding is, as it described, commit 3828fc6 offered a way to load the symbol node_register_module_v${NODE_MODULE_VERSION}. This change should not affect the behavior of an addon defined by macro NODE_MODULE().

[gabrielschulhof]

@fs-eire this change does not affect the behaviour of addons. As far as addons are concerned, there is no change. The cleanup is internal, and only done if the addon is loaded a second time.

[BridgeAR]

Any progress here?

[gabrielschulhof]

@addaleax @bnoordhuis could you please take another look?

[gabrielschulhof]

@addaleax @bnoordhuis could you please take another look?

[addaleax]

@gabrielschulhof Can you explain how to trigger this condition? Ideally also add a test case? That would help a lot

[addaleax]

Shouldn’t it be the reverse situation? Why do we initialize the addon and then close it?

[gabrielschulhof]

We do not close the addon. We close the dlopen() handle, because, if the symbol was already in the cache, we know that we've obtainted a dlopen() handle to it in the past, and thus we can conclude that the dlopen() handle we received this time is not a new handle, the closing of which would cause the addon to be unmapped from our address space, but rather a refcounted version of the old handle we received in the past. Thus, closing the new handle is safe.

[gabrielschulhof]

@addaleax test/addons/hello-world already uses the well-known symbol mechanism, and it loads the addon twice. So, it triggers this condition.

I'm not sure how to produce a testable result as a function of whether dlib.Close() gets called or not, because I don't have a hook by which to feed something back into JS for an assert().

If you run ./node_g test/addons/hello-world/test.js with gdb, step through DLopen() and print dlib before and after CallAddonInitializer() you will see that, during the first call to DLopen(), dlib.handle_ retains its value after CallAddonInitializer() completes, whereas during the second call to DLopen(), calling CallAddonInitalizer() causes dlib.handle_ to become nullptr - meaning that dlib.Close() was called from within CallAddonInitializer().

This ensures that we do not leak libdl refcounts for the addons we load.

[gabrielschulhof]

In retrospect, I believe the correct way of handling multiple dlopen()s of the same module is to allow it, save the dlopen() handle in a private property of the module, and implement module unloading, during the process of which the dlopen() handle will be retrieved from the module's private property in the weak callback, and passed to dlclose().