From 235bb733a6666c4ee625d67f91f08ddeb06ce78d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 9 Mar 2019 18:12:05 +0100 Subject: [PATCH] module: do not share the internal require function with public loaders This patch removes `NativeModule.require` and `NativeModule.requireWithFallbackInDeps`. The public loaders now have to use a special method `NativeModule.prototype.compileForPublicLoader()` to compile native modules. In addition this patch moves the decisions of proxifying exports and throwing unknown builtin errors entirely to public loaders, and skip those during internal use - therefore `loaders.js`, which is compiled during bootstrap, no longer needs to be aware of the value of `--experimental-modules`. PR-URL: https://github.com/nodejs/node/pull/26549 Reviewed-By: James M Snell --- lib/internal/bootstrap/loaders.js | 57 ++++++++++++------------- lib/internal/bootstrap/node.js | 40 +++++++++-------- lib/internal/errors.js | 1 + lib/internal/modules/cjs/loader.js | 9 ++-- lib/internal/modules/esm/translators.js | 9 +++- src/node.cc | 12 +++--- 6 files changed, 65 insertions(+), 63 deletions(-) diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index 28f7f5277b9ee0..d4f70f2270786b 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -19,7 +19,7 @@ // can be created using NODE_MODULE_CONTEXT_AWARE_CPP() with the flag // NM_F_LINKED. // - internalBinding(): the private internal C++ binding loader, inaccessible -// from user land because they are only available from NativeModule.require(). +// from user land unless through `require('internal/test/binding')`. // These C++ bindings are created using NODE_MODULE_CONTEXT_AWARE_INTERNAL() // and have their nm_flags set to NM_F_INTERNAL. // @@ -42,7 +42,7 @@ // This file is compiled as if it's wrapped in a function with arguments // passed by node::RunBootstrapping() /* global process, getLinkedBinding, getInternalBinding */ -/* global experimentalModules, exposeInternals, primordials */ +/* global exposeInternals, primordials */ const { Reflect, @@ -185,27 +185,9 @@ function nativeModuleRequire(id) { } const mod = NativeModule.map.get(id); - if (!mod) { - // Model the error off the internal/errors.js model, but - // do not use that module given that it could actually be - // the one causing the error if there's a bug in Node.js. - // eslint-disable-next-line no-restricted-syntax - const err = new Error(`No such built-in module: ${id}`); - err.code = 'ERR_UNKNOWN_BUILTIN_MODULE'; - err.name = 'Error [ERR_UNKNOWN_BUILTIN_MODULE]'; - throw err; - } - - if (mod.loaded || mod.loading) { - return mod.exports; - } - - moduleLoadList.push(`NativeModule ${id}`); - mod.compile(); - return mod.exports; + return mod.compile(); } -NativeModule.require = nativeModuleRequire; NativeModule.exists = function(id) { return NativeModule.map.has(id); }; @@ -217,11 +199,25 @@ NativeModule.canBeRequiredByUsers = function(id) { // Allow internal modules from dependencies to require // other modules from dependencies by providing fallbacks. -NativeModule.requireWithFallbackInDeps = function(request) { +function requireWithFallbackInDeps(request) { if (!NativeModule.map.has(request)) { request = `internal/deps/${request}`; } - return NativeModule.require(request); + return nativeModuleRequire(request); +} + +// This is exposed for public loaders +NativeModule.prototype.compileForPublicLoader = function(needToProxify) { + if (!this.canBeRequiredByUsers) { + // No code because this is an assertion against bugs + // eslint-disable-next-line no-restricted-syntax + throw new Error(`Should not compile ${this.id} for public use`); + } + this.compile(); + if (needToProxify && !this.exportKeys) { + this.proxifyExports(); + } + return this.exports; }; const getOwn = (target, property, receiver) => { @@ -289,26 +285,27 @@ NativeModule.prototype.proxifyExports = function() { }; NativeModule.prototype.compile = function() { - const id = this.id; + if (this.loaded || this.loading) { + return this.exports; + } + const id = this.id; this.loading = true; try { const requireFn = this.id.startsWith('internal/deps/') ? - NativeModule.requireWithFallbackInDeps : - NativeModule.require; + requireWithFallbackInDeps : nativeModuleRequire; const fn = compileFunction(id); fn(this.exports, requireFn, this, process, internalBinding, primordials); - if (experimentalModules && this.canBeRequiredByUsers) { - this.proxifyExports(); - } - this.loaded = true; } finally { this.loading = false; } + + moduleLoadList.push(`NativeModule ${id}`); + return this.exports; }; // This will be passed to internal/bootstrap/node.js. diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index f4ddc48b4a49e0..e0c220d2a8c3fa 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -33,13 +33,12 @@ // This file is compiled as if it's wrapped in a function with arguments // passed by node::RunBootstrapping() -/* global process, loaderExports, isMainThread, ownsProcessState */ +/* global process, require, internalBinding, isMainThread, ownsProcessState */ /* global primordials */ -const { internalBinding, NativeModule } = loaderExports; const { Object, Symbol } = primordials; const config = internalBinding('config'); -const { deprecate } = NativeModule.require('internal/util'); +const { deprecate } = require('internal/util'); setupProcessObject(); @@ -50,17 +49,17 @@ process.domain = null; process._exiting = false; // Bootstrappers for all threads, including worker threads and main thread -const perThreadSetup = NativeModule.require('internal/process/per_thread'); +const perThreadSetup = require('internal/process/per_thread'); // Bootstrappers for the main thread only let mainThreadSetup; // Bootstrappers for the worker threads only let workerThreadSetup; if (ownsProcessState) { - mainThreadSetup = NativeModule.require( + mainThreadSetup = require( 'internal/process/main_thread_only' ); } else { - workerThreadSetup = NativeModule.require( + workerThreadSetup = require( 'internal/process/worker_thread_only' ); } @@ -116,14 +115,14 @@ if (isMainThread) { const { emitWarning -} = NativeModule.require('internal/process/warning'); +} = require('internal/process/warning'); process.emitWarning = emitWarning; const { nextTick, runNextTicks -} = NativeModule.require('internal/process/next_tick').setup(); +} = require('internal/process/next_tick').setup(); process.nextTick = nextTick; // Used to emulate a tick manually in the JS land. @@ -161,7 +160,7 @@ if (credentials.implementsPosixCredentials) { if (isMainThread) { const { getStdout, getStdin, getStderr } = - NativeModule.require('internal/process/stdio').getMainThreadStdio(); + require('internal/process/stdio').getMainThreadStdio(); setupProcessStdio(getStdout, getStdin, getStderr); } else { const { getStdout, getStdin, getStderr } = @@ -173,7 +172,7 @@ if (isMainThread) { // process. They use the same functions as the JS embedder API. These callbacks // are setup immediately to prevent async_wrap.setupHooks() from being hijacked // and the cost of doing so is negligible. -const { nativeHooks } = NativeModule.require('internal/async_hooks'); +const { nativeHooks } = require('internal/async_hooks'); internalBinding('async_wrap').setupHooks(nativeHooks); // XXX(joyeecheung): this has to be done after the initial load of @@ -183,7 +182,7 @@ if (config.hasInspector) { const { enable, disable - } = NativeModule.require('internal/inspector_async_hook'); + } = require('internal/inspector_async_hook'); internalBinding('inspector').registerAsyncHook(enable, disable); } @@ -193,7 +192,7 @@ if (!config.noBrowserGlobals) { // https://console.spec.whatwg.org/#console-namespace exposeNamespace(global, 'console', createGlobalConsole(global.console)); - const { URL, URLSearchParams } = NativeModule.require('internal/url'); + const { URL, URLSearchParams } = require('internal/url'); // https://url.spec.whatwg.org/#url exposeInterface(global, 'URL', URL); // https://url.spec.whatwg.org/#urlsearchparams @@ -201,14 +200,14 @@ if (!config.noBrowserGlobals) { const { TextEncoder, TextDecoder - } = NativeModule.require('internal/encoding'); + } = require('internal/encoding'); // https://encoding.spec.whatwg.org/#textencoder exposeInterface(global, 'TextEncoder', TextEncoder); // https://encoding.spec.whatwg.org/#textdecoder exposeInterface(global, 'TextDecoder', TextDecoder); // https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope - const timers = NativeModule.require('timers'); + const timers = require('timers'); defineOperation(global, 'clearInterval', timers.clearInterval); defineOperation(global, 'clearTimeout', timers.clearTimeout); defineOperation(global, 'setInterval', timers.setInterval); @@ -302,7 +301,7 @@ Object.defineProperty(process, 'features', { fatalException, setUncaughtExceptionCaptureCallback, hasUncaughtExceptionCaptureCallback - } = NativeModule.require('internal/process/execution'); + } = require('internal/process/execution'); process._fatalException = fatalException; process.setUncaughtExceptionCaptureCallback = @@ -312,7 +311,7 @@ Object.defineProperty(process, 'features', { } function setupProcessObject() { - const EventEmitter = NativeModule.require('events'); + const EventEmitter = require('events'); const origProcProto = Object.getPrototypeOf(process); Object.setPrototypeOf(origProcProto, EventEmitter.prototype); EventEmitter.call(process); @@ -391,7 +390,7 @@ function setupGlobalProxy() { } function setupBuffer() { - const { Buffer } = NativeModule.require('buffer'); + const { Buffer } = require('buffer'); const bufferBinding = internalBinding('buffer'); // Only after this point can C++ use Buffer::New() @@ -404,9 +403,9 @@ function setupBuffer() { function createGlobalConsole(consoleFromVM) { const consoleFromNode = - NativeModule.require('internal/console/global'); + require('internal/console/global'); if (config.hasInspector) { - const inspector = NativeModule.require('internal/util/inspector'); + const inspector = require('internal/util/inspector'); // This will be exposed by `require('inspector').console` later. inspector.consoleFromVM = consoleFromVM; // TODO(joyeecheung): postpone this until the first time inspector @@ -424,8 +423,7 @@ function setupQueueMicrotask() { get() { process.emitWarning('queueMicrotask() is experimental.', 'ExperimentalWarning'); - const { queueMicrotask } = - NativeModule.require('internal/queue_microtask'); + const { queueMicrotask } = require('internal/queue_microtask'); Object.defineProperty(global, 'queueMicrotask', { value: queueMicrotask, diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 30af8f18f4ca2a..3e1921c01340d4 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1017,6 +1017,7 @@ E('ERR_UNHANDLED_ERROR', if (err === undefined) return msg; return `${msg} (${err})`; }, Error); +E('ERR_UNKNOWN_BUILTIN_MODULE', 'No such built-in module: %s', Error); E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error); E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError); diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index ef56540bc43757..eac192e1dc7417 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -567,8 +567,8 @@ Module._resolveLookupPaths = function(request, parent, newReturn) { // Check the cache for the requested file. // 1. If a module already exists in the cache: return its exports object. -// 2. If the module is native: call `NativeModule.require()` with the -// filename and return the result. +// 2. If the module is native: call +// `NativeModule.prototype.compileForPublicLoader()` and return the exports. // 3. Otherwise, create a new module for the file and save it to the cache. // Then have it load the file contents before returning its exports // object. @@ -585,9 +585,10 @@ Module._load = function(request, parent, isMain) { return cachedModule.exports; } - if (NativeModule.canBeRequiredByUsers(filename)) { + const mod = NativeModule.map.get(filename); + if (mod && mod.canBeRequiredByUsers) { debug('load native module %s', request); - return NativeModule.require(filename); + return mod.compileForPublicLoader(experimentalModules); } // Don't call updateChildren(), Module constructor already does. diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index e33fc85993a2b2..97a7f4a2f354f2 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -22,7 +22,9 @@ const { URL } = require('url'); const { debuglog } = require('internal/util/debuglog'); const { promisify } = require('internal/util'); const esmLoader = require('internal/process/esm_loader'); - +const { + ERR_UNKNOWN_BUILTIN_MODULE +} = require('internal/errors').codes; const readFileAsync = promisify(fs.readFile); const readFileSync = fs.readFileSync; const StringReplace = FunctionPrototype.call.bind(StringPrototype.replace); @@ -86,8 +88,11 @@ translators.set('builtin', async (url) => { debug(`Translating BuiltinModule ${url}`); // slice 'node:' scheme const id = url.slice(5); - NativeModule.require(id); const module = NativeModule.map.get(id); + if (!module) { + throw new ERR_UNKNOWN_BUILTIN_MODULE(id); + } + module.compileForPublicLoader(true); return createDynamicModule( [...module.exportKeys, 'default'], url, (reflect) => { debug(`Loading BuiltinModule ${url}`); diff --git a/src/node.cc b/src/node.cc index 3d587ced167821..19e68b13780255 100644 --- a/src/node.cc +++ b/src/node.cc @@ -298,8 +298,6 @@ MaybeLocal RunBootstrapping(Environment* env) { env->process_string(), FIXED_ONE_BYTE_STRING(isolate, "getLinkedBinding"), FIXED_ONE_BYTE_STRING(isolate, "getInternalBinding"), - // --experimental-modules - FIXED_ONE_BYTE_STRING(isolate, "experimentalModules"), // --expose-internals FIXED_ONE_BYTE_STRING(isolate, "exposeInternals"), env->primordials_string()}; @@ -311,7 +309,6 @@ MaybeLocal RunBootstrapping(Environment* env) { env->NewFunctionTemplate(binding::GetInternalBinding) ->GetFunction(context) .ToLocalChecked(), - Boolean::New(isolate, env->options()->experimental_modules), Boolean::New(isolate, env->options()->expose_internals), env->primordials()}; @@ -333,16 +330,19 @@ MaybeLocal RunBootstrapping(Environment* env) { loader_exports_obj->Get(context, env->require_string()).ToLocalChecked(); env->set_native_module_require(require.As()); - // process, loaderExports, isMainThread, ownsProcessState, primordials + // process, require, internalBinding, isMainThread, + // ownsProcessState, primordials std::vector> node_params = { env->process_string(), - FIXED_ONE_BYTE_STRING(isolate, "loaderExports"), + env->require_string(), + env->internal_binding_string(), FIXED_ONE_BYTE_STRING(isolate, "isMainThread"), FIXED_ONE_BYTE_STRING(isolate, "ownsProcessState"), env->primordials_string()}; std::vector> node_args = { process, - loader_exports_obj, + require, + internal_binding_loader, Boolean::New(isolate, env->is_main_thread()), Boolean::New(isolate, env->owns_process_state()), env->primordials()};