module: maintain separate cache for native modules

[vkurchatkin]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

require.cache is often used to "unload" required modules for testing purposes.
This don't quite works for native purposes, because you can't load them twice.
The solution is to maintain independent cache for native modules so we can
create new module instance without invoking dlopen.

Result of realpath is used as cache key to avoid loading same module twice
on case-insensitive file systems.

Fixes: #6160

[vkurchatkin]

A couple of caveats that come to my mind:

• Native module could be explicitly initialised from js, so this could fail if js module is reevaluated;
• exports is attached to one module object, but module initialiser was called with a different module object, so some inconsistencies are possible.

Still it seems better than just failing.

[thefourtheye]

Nit: internalModuleRealPath

[jasnell]

Tentatively marking this as a semver-major due to the nature of change.

[bnoordhuis]

Not a fan. A lot of complexity for something that can be addressed by a one-liner in the documentation.

[vkurchatkin]

@bnoordhuis it also fixes some other cases of loading the same addon twice

[jasnell]

@vkurchatkin ... could those cases be separated out? That is, separate the fix that addresses the addons being loaded twice from the rest of it? For the cache clearing case, I'm wondering if providing a simple require.clearCache() API that removes everything except addons would be enough here? (not 100% sure that even makes sense tho ;-) ...)

[Fishrock123]

@jasnell I'm pretty sure this is that fix.

[vkurchatkin]

@jasnell @Fishrock123 yep, using real path as cache key also solves crash when requiring Foo and foo, and foo has native module inside. Probably there are other similar cases, such as using symlinks

[jasnell]

@Fishrock123 ... sorry, I wasn't clear. I meant (a) separating out the commits and (b) rather than having the separate const nativeModuleCache = {} provide a method on require that removes only the non-native modules from the existing cache.

[vkurchatkin]

provide a method on require that removes only the non-native modules from the existing cache

This won't really fix existing code, so it will work only for users that know about this problem and change their code to avoid it. This is already possible to implement in userland by filtering out paths with .node extension.

[jasnell]

yep, good point. I generally agree with @bnoordhuis with regards to the additional complexity of this but I understand the reasoning behind it. I guess I'm +0 on it for now.

[Fishrock123]

This doesn't really seem all that complex to me. I'm not particularly against it.

[bnoordhuis]

The nature of the module system is such that if we land this change, we're stuck with it forever.

If that doesn't convince you: it's a workaround for bad user expectations. That's what documentation is for.

[vkurchatkin]

bad user expectations

Users expect things to work, if that's what you mean. It's a good thing that we handle some cases that previously resulted in crash, isn't it?

The other option is to deprecate mutating cache altogether. Everything else is a half measure.

[bnoordhuis]

You know what I mean. The OP in #6160 assumed that add-ons could be unloaded by deleting from the cache, but they can't because of technical reasons.

If your position is that we hide that detail with a shadow cache, then that's a valid position to take - but it's one I don't agree with. :-)

[rvagg]

I'm +1 on doing something about this but +0 on this change and unfortunately don't have a better idea just now. While I have sympathy for @bnoordhuis' position I don't agree with "The nature of the module system is such that if we land this change, we're stuck with it forever" on this change, there's no reason we couldn't undo it in another semver-major. It also seems that a lot of the complexity of this change comes from introducing internalModuleRealPath which seems to be addressing a completely separate issue, so are there two issues rolled up in this PR? Should we have separate test cases for those? Perhaps you'd make more headway here if you separate them into two PRs @vkurchatkin?

One concern with this solution is we'd be hiding the cache from users when there are good reasons to expose that data for native modules just like standard modules. An ideal solution here would be to prevent them from deleting cached data for native modules. Perhaps one way of doing this that wouldn't involve a huge perf hit would be to use an internal cache for native modules and each time it's queried, copy the key/value to require.cache, so we maintain an accurate view via require.cache but don't let any edits of the data there to impact on native module loading.