From 8fa90358d7b6a21e945d865f14ddb9ad02572921 Mon Sep 17 00:00:00 2001 From: bcoe Date: Mon, 2 Nov 2020 16:57:33 -0800 Subject: [PATCH] module: refactor to use iterable-weak-map Using an iterable WeakMap (a data-structure that uses WeakRef and WeakMap), we are able to: stop relying on Module._cache to serialize source maps; stop requiring an error object when calling findSourceMap(). PR-URL: https://github.com/nodejs/node/pull/35915 Reviewed-By: Antoine du Hamel Reviewed-By: Matteo Collina --- doc/api/module.md | 13 +-- doc/api/modules.md | 2 +- lib/internal/per_context/primordials.js | 1 + .../source_map/prepare_stack_trace.js | 3 +- lib/internal/source_map/source_map_cache.js | 88 ++++++++--------- lib/internal/util/iterable_weak_map.js | 86 +++++++++++++++++ node.gyp | 1 + .../source-map/throw-on-require-entry.js | 4 + .../source-map/throw-on-require-entry.js.map | 1 + test/fixtures/source-map/throw-on-require.js | 15 +++ .../source-map/throw-on-require.js.map | 1 + test/fixtures/source-map/throw-on-require.ts | 12 +++ test/parallel/test-bootstrap-modules.js | 1 + .../test-internal-iterable-weak-map.js | 95 +++++++++++++++++++ test/parallel/test-source-map-api.js | 2 +- test/parallel/test-source-map-enable.js | 18 ++++ 16 files changed, 281 insertions(+), 62 deletions(-) create mode 100644 lib/internal/util/iterable_weak_map.js create mode 100644 test/fixtures/source-map/throw-on-require-entry.js create mode 100644 test/fixtures/source-map/throw-on-require-entry.js.map create mode 100644 test/fixtures/source-map/throw-on-require.js create mode 100644 test/fixtures/source-map/throw-on-require.js.map create mode 100644 test/fixtures/source-map/throw-on-require.ts create mode 100644 test/parallel/test-internal-iterable-weak-map.js diff --git a/doc/api/module.md b/doc/api/module.md index 2fb6ca884b4..b686adc05f8 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -135,7 +135,10 @@ import { findSourceMap, SourceMap } from 'module'; const { findSourceMap, SourceMap } = require('module'); ``` -### `module.findSourceMap(path[, error])` + + +### `module.findSourceMap(path)` + * `path` {string} -* `error` {Error} * Returns: {module.SourceMap} `path` is the resolved path for the file for which a corresponding source map should be fetched. -The `error` instance should be passed as the second parameter to `findSourceMap` -in exceptional flows, such as when an overridden -[`Error.prepareStackTrace(error, trace)`][] is invoked. Modules are not added to -the module cache until they are successfully loaded. In these cases, source maps -are associated with the `error` instance along with the `path`. - ### Class: `module.SourceMap` -* `module.findSourceMap(path[, error])` +* `module.findSourceMap(path)` * Class: `module.SourceMap` * `new SourceMap(payload)` * `sourceMap.payload` diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 7b17ef8353a..8527e65f2ba 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -86,6 +86,7 @@ function makeSafe(unsafe, safe) { Object.freeze(safe); return safe; } +primordials.makeSafe = makeSafe; // Subclass the constructors because we need to use their prototype // methods later. diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js index 7053a890a8e..267f5dc91a9 100644 --- a/lib/internal/source_map/prepare_stack_trace.js +++ b/lib/internal/source_map/prepare_stack_trace.js @@ -57,7 +57,7 @@ const prepareStackTrace = (globalThis, error, trace) => { let str = i !== 0 ? '\n at ' : ''; str = `${str}${t}`; try { - const sm = findSourceMap(t.getFileName(), error); + const sm = findSourceMap(t.getFileName()); if (sm) { // Source Map V3 lines/columns use zero-based offsets whereas, in // stack traces, they start at 1/1. @@ -119,6 +119,7 @@ function getErrorSource(payload, originalSource, firstLine, firstColumn) { source = readFileSync(originalSourceNoScheme, 'utf8'); } catch (err) { debug(err); + return ''; } } diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index f94ffa8387a..ff259ee4325 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -1,6 +1,7 @@ 'use strict'; const { + ArrayPrototypeMap, JSONParse, ObjectCreate, ObjectKeys, @@ -8,8 +9,10 @@ const { ObjectPrototypeHasOwnProperty, Map, MapPrototypeEntries, - WeakMap, - WeakMapPrototypeGet, + RegExpPrototypeTest, + SafeMap, + StringPrototypeMatch, + StringPrototypeSplit, uncurryThis, } = primordials; @@ -27,17 +30,17 @@ let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => { }); const fs = require('fs'); const { getOptionValue } = require('internal/options'); +const { IterableWeakMap } = require('internal/util/iterable_weak_map'); const { normalizeReferrerURL, } = require('internal/modules/cjs/helpers'); -// For cjs, since Module._cache is exposed to users, we use a WeakMap -// keyed on module, facilitating garbage collection. -const cjsSourceMapCache = new WeakMap(); -// The esm cache is not exposed to users, so we can use a Map keyed -// on filenames. -const esmSourceMapCache = new Map(); -const { fileURLToPath, URL } = require('url'); -let Module; +// Since the CJS module cache is mutable, which leads to memory leaks when +// modules are deleted, we use a WeakMap so that the source map cache will +// be purged automatically: +const cjsSourceMapCache = new IterableWeakMap(); +// The esm cache is not mutable, so we can use a Map without memory concerns: +const esmSourceMapCache = new SafeMap(); +const { fileURLToPath, pathToFileURL, URL } = require('internal/url'); let SourceMap; let sourceMapsEnabled; @@ -70,13 +73,14 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) { debug(err.stack); return; } - - const match = content.match(/\/[*/]#\s+sourceMappingURL=(?[^\s]+)/); + const match = StringPrototypeMatch( + content, + /\/[*/]#\s+sourceMappingURL=(?[^\s]+)/ + ); if (match) { const data = dataFromUrl(filename, match.groups.sourceMappingURL); const url = data ? null : match.groups.sourceMappingURL; if (cjsModuleInstance) { - if (!Module) Module = require('internal/modules/cjs/loader').Module; cjsSourceMapCache.set(cjsModuleInstance, { filename, lineLengths: lineLengths(content), @@ -120,7 +124,7 @@ function lineLengths(content) { // We purposefully keep \r as part of the line-length calculation, in // cases where there is a \r\n separator, so that this can be taken into // account in coverage calculations. - return content.split(/\n|\u2028|\u2029/).map((line) => { + return ArrayPrototypeMap(StringPrototypeSplit(content, /\n|\u2028|\u2029/), (line) => { return line.length; }); } @@ -139,8 +143,8 @@ function sourceMapFromFile(mapURL) { // data:[][;base64], see: // https://tools.ietf.org/html/rfc2397#section-2 function sourceMapFromDataUrl(sourceURL, url) { - const [format, data] = url.split(','); - const splitFormat = format.split(';'); + const [format, data] = StringPrototypeSplit(url, ','); + const splitFormat = StringPrototypeSplit(format, ';'); const contentType = splitFormat[0]; const base64 = splitFormat[splitFormat.length - 1] === 'base64'; if (contentType === 'application/json') { @@ -207,48 +211,32 @@ function sourceMapCacheToObject() { return obj; } -// Since WeakMap can't be iterated over, we use Module._cache's -// keys to facilitate Source Map serialization. -// -// TODO(bcoe): this means we don't currently serialize source-maps attached -// to error instances, only module instances. function appendCJSCache(obj) { - if (!Module) return; - const cjsModuleCache = ObjectGetValueSafe(Module, '_cache'); - const cjsModules = ObjectKeys(cjsModuleCache); - for (let i = 0; i < cjsModules.length; i++) { - const key = cjsModules[i]; - const module = ObjectGetValueSafe(cjsModuleCache, key); - const value = WeakMapPrototypeGet(cjsSourceMapCache, module); - if (value) { - // This is okay because `obj` has a null prototype. - obj[`file://${key}`] = { - lineLengths: ObjectGetValueSafe(value, 'lineLengths'), - data: ObjectGetValueSafe(value, 'data'), - url: ObjectGetValueSafe(value, 'url') - }; - } + for (const value of cjsSourceMapCache) { + obj[ObjectGetValueSafe(value, 'filename')] = { + lineLengths: ObjectGetValueSafe(value, 'lineLengths'), + data: ObjectGetValueSafe(value, 'data'), + url: ObjectGetValueSafe(value, 'url') + }; } } -// Attempt to lookup a source map, which is either attached to a file URI, or -// keyed on an error instance. -// TODO(bcoe): once WeakRefs are available in Node.js, refactor to drop -// requirement of error parameter. -function findSourceMap(uri, error) { - if (!Module) Module = require('internal/modules/cjs/loader').Module; +function findSourceMap(sourceURL) { + if (!RegExpPrototypeTest(/^\w+:\/\//, sourceURL)) { + sourceURL = pathToFileURL(sourceURL).href; + } if (!SourceMap) { SourceMap = require('internal/source_map/source_map').SourceMap; } - let sourceMap = cjsSourceMapCache.get(Module._cache[uri]); - if (!uri.startsWith('file://')) uri = normalizeReferrerURL(uri); + let sourceMap = esmSourceMapCache.get(sourceURL); if (sourceMap === undefined) { - sourceMap = esmSourceMapCache.get(uri); - } - if (sourceMap === undefined) { - const candidateSourceMap = cjsSourceMapCache.get(error); - if (candidateSourceMap && uri === candidateSourceMap.filename) { - sourceMap = candidateSourceMap; + for (const value of cjsSourceMapCache) { + const filename = ObjectGetValueSafe(value, 'filename'); + if (sourceURL === filename) { + sourceMap = { + data: ObjectGetValueSafe(value, 'data') + }; + } } } if (sourceMap && sourceMap.data) { diff --git a/lib/internal/util/iterable_weak_map.js b/lib/internal/util/iterable_weak_map.js new file mode 100644 index 00000000000..3fa139b23e4 --- /dev/null +++ b/lib/internal/util/iterable_weak_map.js @@ -0,0 +1,86 @@ +'use strict'; + +const { + makeSafe, + Object, + SafeSet, + SafeWeakMap, + SymbolIterator, +} = primordials; + +// TODO(aduh95): Add FinalizationRegistry to primordials +const SafeFinalizationRegistry = makeSafe( + globalThis.FinalizationRegistry, + class SafeFinalizationRegistry extends globalThis.FinalizationRegistry {} +); + +// TODO(aduh95): Add WeakRef to primordials +const SafeWeakRef = makeSafe( + globalThis.WeakRef, + class SafeWeakRef extends globalThis.WeakRef {} +); + +// This class is modified from the example code in the WeakRefs specification: +// https://github.com/tc39/proposal-weakrefs +// Licensed under ECMA's MIT-style license, see: +// https://github.com/tc39/ecma262/blob/master/LICENSE.md +class IterableWeakMap { + #weakMap = new SafeWeakMap(); + #refSet = new SafeSet(); + #finalizationGroup = new SafeFinalizationRegistry(cleanup); + + set(key, value) { + const entry = this.#weakMap.get(key); + if (entry) { + // If there's already an entry for the object represented by "key", + // the value can be updated without creating a new WeakRef: + this.#weakMap.set(key, { value, ref: entry.ref }); + } else { + const ref = new SafeWeakRef(key); + this.#weakMap.set(key, { value, ref }); + this.#refSet.add(ref); + this.#finalizationGroup.register(key, { + set: this.#refSet, + ref + }, ref); + } + } + + get(key) { + return this.#weakMap.get(key)?.value; + } + + has(key) { + return this.#weakMap.has(key); + } + + delete(key) { + const entry = this.#weakMap.get(key); + if (!entry) { + return false; + } + this.#weakMap.delete(key); + this.#refSet.delete(entry.ref); + this.#finalizationGroup.unregister(entry.ref); + return true; + } + + *[SymbolIterator]() { + for (const ref of this.#refSet) { + const key = ref.deref(); + if (!key) continue; + const { value } = this.#weakMap.get(key); + yield value; + } + } +} + +function cleanup({ set, ref }) { + set.delete(ref); +} + +Object.freeze(IterableWeakMap.prototype); + +module.exports = { + IterableWeakMap, +}; diff --git a/node.gyp b/node.gyp index d2f0be54e93..643c8a43c6f 100644 --- a/node.gyp +++ b/node.gyp @@ -230,6 +230,7 @@ 'lib/internal/util/debuglog.js', 'lib/internal/util/inspect.js', 'lib/internal/util/inspector.js', + 'lib/internal/util/iterable_weak_map.js', 'lib/internal/util/types.js', 'lib/internal/http2/core.js', 'lib/internal/http2/compat.js', diff --git a/test/fixtures/source-map/throw-on-require-entry.js b/test/fixtures/source-map/throw-on-require-entry.js new file mode 100644 index 00000000000..94b14810a52 --- /dev/null +++ b/test/fixtures/source-map/throw-on-require-entry.js @@ -0,0 +1,4 @@ +"use strict"; +exports.__esModule = true; +require("./throw-on-require"); +//# sourceMappingURL=throw-on-require-entry.js.map diff --git a/test/fixtures/source-map/throw-on-require-entry.js.map b/test/fixtures/source-map/throw-on-require-entry.js.map new file mode 100644 index 00000000000..a5f0ca2253f --- /dev/null +++ b/test/fixtures/source-map/throw-on-require-entry.js.map @@ -0,0 +1 @@ +{"version":3,"file":"throw-on-require-entry.js","sourceRoot":"","sources":["throw-on-require-entry.ts"],"names":[],"mappings":";;AAAA,8BAA2B"} \ No newline at end of file diff --git a/test/fixtures/source-map/throw-on-require.js b/test/fixtures/source-map/throw-on-require.js new file mode 100644 index 00000000000..5654f2bfb91 --- /dev/null +++ b/test/fixtures/source-map/throw-on-require.js @@ -0,0 +1,15 @@ +var ATrue; +(function (ATrue) { + ATrue[ATrue["IsTrue"] = 1] = "IsTrue"; + ATrue[ATrue["IsFalse"] = 0] = "IsFalse"; +})(ATrue || (ATrue = {})); +if (false) { + console.info('unreachable'); +} +else if (true) { + throw Error('throw early'); +} +else { + console.info('unreachable'); +} +//# sourceMappingURL=throw-on-require.js.map \ No newline at end of file diff --git a/test/fixtures/source-map/throw-on-require.js.map b/test/fixtures/source-map/throw-on-require.js.map new file mode 100644 index 00000000000..94d0df9c03a --- /dev/null +++ b/test/fixtures/source-map/throw-on-require.js.map @@ -0,0 +1 @@ +{"version":3,"file":"throw-on-require.js","sourceRoot":"","sources":["throw-on-require.ts"],"names":[],"mappings":"AAAA,IAAK,KAGJ;AAHD,WAAK,KAAK;IACR,qCAAU,CAAA;IACV,uCAAW,CAAA;AACb,CAAC,EAHI,KAAK,KAAL,KAAK,QAGT;AAED,IAAI,KAAK,EAAE;IACT,OAAO,CAAC,IAAI,CAAC,aAAa,CAAC,CAAA;CAC5B;KAAM,IAAI,IAAI,EAAE;IACf,MAAM,KAAK,CAAC,aAAa,CAAC,CAAA;CAC3B;KAAM;IACL,OAAO,CAAC,IAAI,CAAC,aAAa,CAAC,CAAA;CAC5B"} \ No newline at end of file diff --git a/test/fixtures/source-map/throw-on-require.ts b/test/fixtures/source-map/throw-on-require.ts new file mode 100644 index 00000000000..4aa57030277 --- /dev/null +++ b/test/fixtures/source-map/throw-on-require.ts @@ -0,0 +1,12 @@ +enum ATrue { + IsTrue = 1, + IsFalse = 0 +} + +if (false) { + console.info('unreachable') +} else if (true) { + throw Error('throw early') +} else { + console.info('unreachable') +} diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 2a811d65cc2..b9844dc4caf 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -92,6 +92,7 @@ const expectedModules = new Set([ 'NativeModule internal/util', 'NativeModule internal/util/debuglog', 'NativeModule internal/util/inspect', + 'NativeModule internal/util/iterable_weak_map', 'NativeModule internal/util/types', 'NativeModule internal/validators', 'NativeModule internal/vm/module', diff --git a/test/parallel/test-internal-iterable-weak-map.js b/test/parallel/test-internal-iterable-weak-map.js new file mode 100644 index 00000000000..04636c20903 --- /dev/null +++ b/test/parallel/test-internal-iterable-weak-map.js @@ -0,0 +1,95 @@ +// Flags: --expose-gc --expose-internals +'use strict'; + +require('../common'); +const { deepStrictEqual, strictEqual } = require('assert'); +const { IterableWeakMap } = require('internal/util/iterable_weak_map'); + +// It drops entry if a reference is no longer held. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + moduleB: {}, + moduleC: {}, + }; + wm.set(_cache.moduleA, 'hello'); + wm.set(_cache.moduleB, 'discard'); + wm.set(_cache.moduleC, 'goodbye'); + delete _cache.moduleB; + setImmediate(() => { + _cache; + globalThis.gc(); + const values = [...wm]; + deepStrictEqual(values, ['hello', 'goodbye']); + }); +} + +// It updates an existing entry, if the same key is provided twice. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + moduleB: {}, + }; + wm.set(_cache.moduleA, 'hello'); + wm.set(_cache.moduleB, 'goodbye'); + wm.set(_cache.moduleB, 'goodnight'); + const values = [...wm]; + deepStrictEqual(values, ['hello', 'goodnight']); +} + +// It allows entry to be deleted by key. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + moduleB: {}, + moduleC: {}, + }; + wm.set(_cache.moduleA, 'hello'); + wm.set(_cache.moduleB, 'discard'); + wm.set(_cache.moduleC, 'goodbye'); + wm.delete(_cache.moduleB); + const values = [...wm]; + deepStrictEqual(values, ['hello', 'goodbye']); +} + +// It handles delete for key that does not exist. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + moduleB: {}, + moduleC: {}, + }; + wm.set(_cache.moduleA, 'hello'); + wm.set(_cache.moduleC, 'goodbye'); + wm.delete(_cache.moduleB); + const values = [...wm]; + deepStrictEqual(values, ['hello', 'goodbye']); +} + +// It allows an entry to be fetched by key. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + moduleB: {}, + moduleC: {}, + }; + wm.set(_cache.moduleA, 'hello'); + wm.set(_cache.moduleB, 'discard'); + wm.set(_cache.moduleC, 'goodbye'); + strictEqual(wm.get(_cache.moduleB), 'discard'); +} + +// It returns true for has() if key exists. +{ + const wm = new IterableWeakMap(); + const _cache = { + moduleA: {}, + }; + wm.set(_cache.moduleA, 'hello'); + strictEqual(wm.has(_cache.moduleA), true); +} diff --git a/test/parallel/test-source-map-api.js b/test/parallel/test-source-map-api.js index 60bbb661e1c..1a3d180619f 100644 --- a/test/parallel/test-source-map-api.js +++ b/test/parallel/test-source-map-api.js @@ -31,7 +31,7 @@ const { readFileSync } = require('fs'); Error.prepareStackTrace = (error, trace) => { const throwingRequireCallSite = trace[0]; if (throwingRequireCallSite.getFileName().endsWith('typescript-throw.js')) { - sourceMap = findSourceMap(throwingRequireCallSite.getFileName(), error); + sourceMap = findSourceMap(throwingRequireCallSite.getFileName()); callSite = throwingRequireCallSite; } }; diff --git a/test/parallel/test-source-map-enable.js b/test/parallel/test-source-map-enable.js index 0887ae8811c..cea597b4f7b 100644 --- a/test/parallel/test-source-map-enable.js +++ b/test/parallel/test-source-map-enable.js @@ -283,6 +283,24 @@ function nextdir() { assert.ok(output.stderr.toString().includes('webpack:///webpack.js:14:9')); } +// Stores and applies source map associated with file that throws while +// being required. +{ + const coverageDirectory = nextdir(); + const output = spawnSync(process.execPath, [ + '--enable-source-maps', + require.resolve('../fixtures/source-map/throw-on-require-entry.js') + ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + const sourceMap = getSourceMapFromCache( + 'throw-on-require.js', + coverageDirectory + ); + // Rewritten stack trace. + assert.match(output.stderr.toString(), /throw-on-require\.ts:9:9/); + // Source map should have been serialized. + assert.ok(sourceMap); +} + function getSourceMapFromCache(fixtureFile, coverageDirectory) { const jsonFiles = fs.readdirSync(coverageDirectory); for (const jsonFile of jsonFiles) {