From df42c60ee6e4416217b341d1bb51e94b48e1639b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 9 Mar 2019 18:12:05 +0100 Subject: [PATCH] modules: 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`. --- lib/internal/bootstrap/loaders.js | 57 ++++++++++++------------- lib/internal/bootstrap/node.js | 42 +++++++++--------- 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, 67 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 f8cddb3153ed20..3e09eec54f897d 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,18 @@ if (isMainThread) { const { emitWarning -} = NativeModule.require('internal/process/warning'); +} = require('internal/process/warning'); process.emitWarning = emitWarning; +const { + setupTaskQueue, + queueMicrotask +} = require('internal/process/task_queues'); const { nextTick, runNextTicks -} = NativeModule.require('internal/process/task_queues').setupTaskQueue(); +} = setupTaskQueue(); process.nextTick = nextTick; // Used to emulate a tick manually in the JS land. @@ -161,7 +164,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 +176,7 @@ if (config.hasInspector) { const { enable, disable - } = NativeModule.require('internal/inspector_async_hook'); + } = require('internal/inspector_async_hook'); internalBinding('inspector').registerAsyncHook(enable, disable); } @@ -183,7 +186,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 @@ -191,22 +194,19 @@ 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); defineOperation(global, 'setTimeout', timers.setTimeout); - const { - queueMicrotask - } = NativeModule.require('internal/process/task_queues'); defineOperation(global, 'queueMicrotask', queueMicrotask); // Non-standard extensions: @@ -265,7 +265,7 @@ Object.defineProperty(process, 'features', { fatalException, setUncaughtExceptionCaptureCallback, hasUncaughtExceptionCaptureCallback - } = NativeModule.require('internal/process/execution'); + } = require('internal/process/execution'); process._fatalException = fatalException; process.setUncaughtExceptionCaptureCallback = @@ -275,7 +275,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); @@ -359,7 +359,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() @@ -377,9 +377,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 diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a76153297ecd69..7b00bed3168a40 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -967,6 +967,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 6bd7f7753517ff..ee1c814c814bd5 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -568,8 +568,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. @@ -586,9 +586,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 25552cff0e8f47..cf1765c7c3bd60 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -21,7 +21,9 @@ const { const { URL } = require('url'); const { debuglog, promisify } = require('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); @@ -85,8 +87,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 783962d192f5f0..ccff5b6aa4bb9e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -296,8 +296,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()}; @@ -309,7 +307,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()}; @@ -331,16 +328,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()};