From 2496db8e0946de58911cac7b495d854cb18c179d Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 26 Apr 2020 21:40:01 -0700 Subject: [PATCH 01/13] module: no type module resolver side effects PR-URL: https://github.com/nodejs/node/pull/33086 Reviewed-By: Jan Krems Reviewed-By: Geoffrey Booth --- doc/api/esm.md | 7 ------- lib/internal/modules/esm/resolve.js | 9 +-------- test/es-module/test-esm-type-main.mjs | 9 +++++++++ test/fixtures/node_modules/type-main/index.js | 1 + test/fixtures/node_modules/type-main/package.json | 4 ++++ 5 files changed, 15 insertions(+), 15 deletions(-) create mode 100644 test/es-module/test-esm-type-main.mjs create mode 100644 test/fixtures/node_modules/type-main/index.js create mode 100644 test/fixtures/node_modules/type-main/package.json diff --git a/doc/api/esm.md b/doc/api/esm.md index 49c467effbc3cd..5047600f988f66 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1601,13 +1601,6 @@ The resolver can throw the following errors: > 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, > _mainExport_, _""_). > 1. Throw a _Package Path Not Exported_ error. -> 1. If _pjson.main_ is a String, then -> 1. Let _resolvedMain_ be the URL resolution of _packageURL_, "/", and -> _pjson.main_. -> 1. If the file at _resolvedMain_ exists, then -> 1. Return _resolvedMain_. -> 1. If _pjson.type_ is equal to _"module"_, then -> 1. Throw a _Module Not Found_ error. > 1. Let _legacyMainURL_ be the result applying the legacy > **LOAD_AS_DIRECTORY** CommonJS resolver to _packageURL_, throwing a > _Module Not Found_ error for no resolution. diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 5bb2f37e53eeb5..8b8582544494a5 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -439,11 +439,6 @@ function packageMainResolve(packageJSONUrl, packageConfig, base, conditions) { throw new ERR_PACKAGE_PATH_NOT_EXPORTED(packageJSONUrl, '.'); } - if (packageConfig.main !== undefined) { - const resolved = new URL(packageConfig.main, packageJSONUrl); - const path = fileURLToPath(resolved); - if (tryStatSync(path).isFile()) return resolved; - } if (getOptionValue('--experimental-specifier-resolution') === 'node') { if (packageConfig.main !== undefined) { return finalizeResolution( @@ -453,9 +448,7 @@ function packageMainResolve(packageJSONUrl, packageConfig, base, conditions) { new URL('index', packageJSONUrl), base); } } - if (packageConfig.type !== 'module') { - return legacyMainResolve(packageJSONUrl, packageConfig); - } + return legacyMainResolve(packageJSONUrl, packageConfig); } throw new ERR_MODULE_NOT_FOUND( diff --git a/test/es-module/test-esm-type-main.mjs b/test/es-module/test-esm-type-main.mjs new file mode 100644 index 00000000000000..012cf4f35fc959 --- /dev/null +++ b/test/es-module/test-esm-type-main.mjs @@ -0,0 +1,9 @@ +import { mustNotCall } from '../common/index.mjs'; +import assert from 'assert'; +import { importFixture } from '../fixtures/pkgexports.mjs'; + +(async () => { + const m = await importFixture('type-main'); + assert.strictEqual(m.default, 'asdf'); +})() +.catch(mustNotCall); diff --git a/test/fixtures/node_modules/type-main/index.js b/test/fixtures/node_modules/type-main/index.js new file mode 100644 index 00000000000000..a4cb53964349f5 --- /dev/null +++ b/test/fixtures/node_modules/type-main/index.js @@ -0,0 +1 @@ +export default 'asdf'; diff --git a/test/fixtures/node_modules/type-main/package.json b/test/fixtures/node_modules/type-main/package.json new file mode 100644 index 00000000000000..7665d7a8037428 --- /dev/null +++ b/test/fixtures/node_modules/type-main/package.json @@ -0,0 +1,4 @@ +{ + "main": "index", + "type": "module" +} From 0bd24a6e563f92e67f582dbd470355acc74f7792 Mon Sep 17 00:00:00 2001 From: himself65 Date: Fri, 17 Apr 2020 15:04:38 +0800 Subject: [PATCH 02/13] stream: simplify Readable push/unshift logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/32899 Reviewed-By: Robert Nagy Reviewed-By: Luigi Pinca Reviewed-By: Ruben Bridgewater Reviewed-By: Juan José Arboleda Reviewed-By: James M Snell --- lib/_stream_readable.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index c4758fb7a9f6d6..aac65e9cb30298 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -233,13 +233,15 @@ function readableAddChunk(stream, chunk, encoding, addToFront) { if (!state.objectMode) { if (typeof chunk === 'string') { encoding = encoding || state.defaultEncoding; - if (addToFront && state.encoding && state.encoding !== encoding) { - // When unshifting, if state.encoding is set, we have to save - // the string in the BufferList with the state encoding. - chunk = Buffer.from(chunk, encoding).toString(state.encoding); - } else if (encoding !== state.encoding) { - chunk = Buffer.from(chunk, encoding); - encoding = ''; + if (state.encoding !== encoding) { + if (addToFront && state.encoding) { + // When unshifting, if state.encoding is set, we have to save + // the string in the BufferList with the state encoding. + chunk = Buffer.from(chunk, encoding).toString(state.encoding); + } else { + chunk = Buffer.from(chunk, encoding); + encoding = ''; + } } } else if (chunk instanceof Buffer) { encoding = ''; From 5f2c4ce74f33cf7da5628f6a28c2c06841e16c90 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 22 Apr 2020 10:28:20 +0800 Subject: [PATCH 03/13] vm: fix vm.measureMemory() and introduce execution option https://github.com/nodejs/node-v8/pull/147 broke the `vm.measureMemory()` API. It only created a `MeasureMemoryDelegate` and without actually calling `v8::Isolate::MeasureMemory()` so the returned promise will never resolve. This was not caught by the tests because the promise resolvers were not wrapped with `common.mustCall()`. This patch migrates the API properly and also introduce the newly added execution option to the API. It also removes support for specifying contexts to measure - instead we'll just return the measurements for all contexts in the detailed mode, which is what the `performance.measureMemory()` prototype in V8 currently does. We can consider implementing our own `v8::MeasureMemoryDelegate` to select the target context in `ShouldMeasure()` in the future, but then we'll also need to implement `MeasurementComplete()` to assemble the result. For now it's probably too early to do that. Since this API is still experimental (and guarded with a warning), such breakage should be acceptable. Refs: https://github.com/nodejs/node-v8/pull/147 PR-URL: https://github.com/nodejs/node/pull/32988 Reviewed-By: Ben Noordhuis Reviewed-By: Jiawen Geng Reviewed-By: James M Snell --- doc/api/vm.md | 62 ++++++++++++------ lib/vm.js | 17 +++-- src/node_contextify.cc | 58 +++++++++-------- test/common/measure-memory.js | 57 +++++++++++++++++ test/parallel/test-vm-measure-memory-lazy.js | 37 +++++++++++ .../test-vm-measure-memory-multi-context.js | 28 ++++++++ test/parallel/test-vm-measure-memory.js | 64 +++++-------------- 7 files changed, 226 insertions(+), 97 deletions(-) create mode 100644 test/common/measure-memory.js create mode 100644 test/parallel/test-vm-measure-memory-lazy.js create mode 100644 test/parallel/test-vm-measure-memory-multi-context.js diff --git a/doc/api/vm.md b/doc/api/vm.md index 7957c9bb3e51bd..5bf0c6e62b75d9 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -303,15 +303,21 @@ added: v13.10.0 > Stability: 1 - Experimental -Measure the memory known to V8 and used by the current execution context -or a specified context. +Measure the memory known to V8 and used by all contexts known to the +current V8 isolate, or the main context. * `options` {Object} Optional. - * `mode` {string} Either `'summary'` or `'detailed'`. + * `mode` {string} Either `'summary'` or `'detailed'`. In summary mode, + only the memory measured for the main context will be returned. In + detailed mode, the measure measured for all contexts known to the + current V8 isolate will be returned. **Default:** `'summary'` - * `context` {Object} Optional. A [contextified][] object returned - by `vm.createContext()`. If not specified, measure the memory - usage of the current context where `vm.measureMemory()` is invoked. + * `execution` {string} Either `'default'` or `'eager'`. With default + execution, the promise will not resolve until after the next scheduled + garbage collection starts, which may take a while (or never if the program + exits before the next GC). With eager execution, the GC will be started + right away to measure the memory. + **Default:** `'default'` * Returns: {Promise} If the memory is successfully measured the promise will resolve with an object containing information about the memory usage. @@ -319,28 +325,48 @@ The format of the object that the returned Promise may resolve with is specific to the V8 engine and may change from one version of V8 to the next. The returned result is different from the statistics returned by -`v8.getHeapSpaceStatistics()` in that `vm.measureMemory()` measures -the memory reachable by V8 from a specific context, while -`v8.getHeapSpaceStatistics()` measures the memory used by an instance -of V8 engine, which can switch among multiple contexts that reference -objects in the heap of one engine. +`v8.getHeapSpaceStatistics()` in that `vm.measureMemory()` measure the +memory reachable by each V8 specific contexts in the current instance of +the V8 engine, while the result of `v8.getHeapSpaceStatistics()` measure +the memory occupied by each heap space in the current V8 instance. ```js const vm = require('vm'); -// Measure the memory used by the current context and return the result -// in summary. +// Measure the memory used by the main context. vm.measureMemory({ mode: 'summary' }) - // Is the same as vm.measureMemory() + // This is the same as vm.measureMemory() .then((result) => { // The current format is: - // { total: { jsMemoryEstimate: 2211728, jsMemoryRange: [ 0, 2211728 ] } } + // { + // total: { + // jsMemoryEstimate: 2418479, jsMemoryRange: [ 2418479, 2745799 ] + // } + // } console.log(result); }); -const context = vm.createContext({}); -vm.measureMemory({ mode: 'detailed' }, context) +const context = vm.createContext({ a: 1 }); +vm.measureMemory({ mode: 'detailed', execution: 'eager' }) .then((result) => { - // At the moment the detailed format is the same as the summary one. + // Reference the context here so that it won't be GC'ed + // until the measurement is complete. + console.log(context.a); + // { + // total: { + // jsMemoryEstimate: 2574732, + // jsMemoryRange: [ 2574732, 2904372 ] + // }, + // current: { + // jsMemoryEstimate: 2438996, + // jsMemoryRange: [ 2438996, 2768636 ] + // }, + // other: [ + // { + // jsMemoryEstimate: 135736, + // jsMemoryRange: [ 135736, 465376 ] + // } + // ] + // } console.log(result); }); ``` diff --git a/lib/vm.js b/lib/vm.js index cffca355720dae..fd81e9da8b0515 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -385,20 +385,27 @@ const measureMemoryModes = { detailed: constants.measureMemory.mode.DETAILED, }; +const measureMemoryExecutions = { + default: constants.measureMemory.execution.DEFAULT, + eager: constants.measureMemory.execution.EAGER, +}; + function measureMemory(options = {}) { emitExperimentalWarning('vm.measureMemory'); validateObject(options, 'options'); - const { mode = 'summary', context } = options; + const { mode = 'summary', execution = 'default' } = options; if (mode !== 'summary' && mode !== 'detailed') { throw new ERR_INVALID_ARG_VALUE( 'options.mode', options.mode, 'must be either \'summary\' or \'detailed\''); } - if (context !== undefined && - (typeof context !== 'object' || context === null || !_isContext(context))) { - throw new ERR_INVALID_ARG_TYPE('options.context', 'vm.Context', context); + if (execution !== 'default' && execution !== 'eager') { + throw new ERR_INVALID_ARG_VALUE( + 'options.execution', options.execution, + 'must be either \'default\' or \'eager\''); } - const result = _measureMemory(measureMemoryModes[mode], context); + const result = _measureMemory(measureMemoryModes[mode], + measureMemoryExecutions[execution]); if (result === undefined) { return PromiseReject(new ERR_CONTEXT_NOT_INITIALIZED()); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 34dbdf08420329..99adccbcef9d64 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -53,6 +53,7 @@ using v8::Isolate; using v8::Local; using v8::Maybe; using v8::MaybeLocal; +using v8::MeasureMemoryExecution; using v8::MeasureMemoryMode; using v8::Name; using v8::NamedPropertyHandlerConfiguration; @@ -1211,29 +1212,22 @@ static void WatchdogHasPendingSigint(const FunctionCallbackInfo& args) { static void MeasureMemory(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsInt32()); int32_t mode = args[0].As()->Value(); + int32_t execution = args[1].As()->Value(); Isolate* isolate = args.GetIsolate(); - Environment* env = Environment::GetCurrent(args); - Local context; - if (args[1]->IsUndefined()) { - context = isolate->GetCurrentContext(); - } else { - CHECK(args[1]->IsObject()); - ContextifyContext* sandbox = - ContextifyContext::ContextFromContextifiedSandbox(env, - args[1].As()); - CHECK_NOT_NULL(sandbox); - context = sandbox->context(); - if (context.IsEmpty()) { // Not yet fully initialized - return; - } - } + + Local current_context = isolate->GetCurrentContext(); Local resolver; - if (!Promise::Resolver::New(context).ToLocal(&resolver)) return; - std::unique_ptr i = + if (!Promise::Resolver::New(current_context).ToLocal(&resolver)) return; + std::unique_ptr delegate = v8::MeasureMemoryDelegate::Default( - isolate, context, resolver, static_cast(mode)); - CHECK_NOT_NULL(i); + isolate, + current_context, + resolver, + static_cast(mode)); + isolate->MeasureMemory(std::move(delegate), + static_cast(execution)); v8::Local promise = resolver->GetPromise(); args.GetReturnValue().Set(promise); @@ -1265,13 +1259,27 @@ void Initialize(Local target, Local constants = Object::New(env->isolate()); Local measure_memory = Object::New(env->isolate()); - Local memory_mode = Object::New(env->isolate()); - MeasureMemoryMode SUMMARY = MeasureMemoryMode::kSummary; - MeasureMemoryMode DETAILED = MeasureMemoryMode::kDetailed; - NODE_DEFINE_CONSTANT(memory_mode, SUMMARY); - NODE_DEFINE_CONSTANT(memory_mode, DETAILED); - READONLY_PROPERTY(measure_memory, "mode", memory_mode); + Local memory_execution = Object::New(env->isolate()); + + { + Local memory_mode = Object::New(env->isolate()); + MeasureMemoryMode SUMMARY = MeasureMemoryMode::kSummary; + MeasureMemoryMode DETAILED = MeasureMemoryMode::kDetailed; + NODE_DEFINE_CONSTANT(memory_mode, SUMMARY); + NODE_DEFINE_CONSTANT(memory_mode, DETAILED); + READONLY_PROPERTY(measure_memory, "mode", memory_mode); + } + + { + MeasureMemoryExecution DEFAULT = MeasureMemoryExecution::kDefault; + MeasureMemoryExecution EAGER = MeasureMemoryExecution::kEager; + NODE_DEFINE_CONSTANT(memory_execution, DEFAULT); + NODE_DEFINE_CONSTANT(memory_execution, EAGER); + READONLY_PROPERTY(measure_memory, "execution", memory_execution); + } + READONLY_PROPERTY(constants, "measureMemory", measure_memory); + target->Set(context, env->constants_string(), constants).Check(); env->SetMethod(target, "measureMemory", MeasureMemory); diff --git a/test/common/measure-memory.js b/test/common/measure-memory.js new file mode 100644 index 00000000000000..67c8fb3b9d2d67 --- /dev/null +++ b/test/common/measure-memory.js @@ -0,0 +1,57 @@ +/* eslint-disable node-core/require-common-first, node-core/required-modules */ +'use strict'; + +const assert = require('assert'); +const common = require('./'); + +// The formats could change when V8 is updated, then the tests should be +// updated accordingly. +function assertResultShape(result) { + assert.strictEqual(typeof result.jsMemoryEstimate, 'number'); + assert.strictEqual(typeof result.jsMemoryRange[0], 'number'); + assert.strictEqual(typeof result.jsMemoryRange[1], 'number'); +} + +function assertSummaryShape(result) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assertResultShape(result.total); +} + +function assertDetailedShape(result, contexts = 0) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assert.strictEqual(typeof result.current, 'object'); + assertResultShape(result.total); + assertResultShape(result.current); + if (contexts === 0) { + assert.deepStrictEqual(result.other, []); + } else { + assert.strictEqual(result.other.length, contexts); + for (const item of result.other) { + assertResultShape(item); + } + } +} + +function assertSingleDetailedShape(result) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assert.strictEqual(typeof result.current, 'object'); + assert.deepStrictEqual(result.other, []); + assertResultShape(result.total); + assertResultShape(result.current); +} + +function expectExperimentalWarning() { + common.expectWarning('ExperimentalWarning', + 'vm.measureMemory is an experimental feature. ' + + 'This feature could change at any time'); +} + +module.exports = { + assertSummaryShape, + assertDetailedShape, + assertSingleDetailedShape, + expectExperimentalWarning +}; diff --git a/test/parallel/test-vm-measure-memory-lazy.js b/test/parallel/test-vm-measure-memory-lazy.js new file mode 100644 index 00000000000000..513cfbc3672451 --- /dev/null +++ b/test/parallel/test-vm-measure-memory-lazy.js @@ -0,0 +1,37 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../common'); +const { + assertSummaryShape, + expectExperimentalWarning +} = require('../common/measure-memory'); +const vm = require('vm'); + +expectExperimentalWarning(); + +// Test lazy memory measurement - we will need to global.gc() +// or otherwise these may not resolve. +{ + vm.measureMemory() + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({}) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({ mode: 'summary' }) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({ mode: 'detailed' }) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} diff --git a/test/parallel/test-vm-measure-memory-multi-context.js b/test/parallel/test-vm-measure-memory-multi-context.js new file mode 100644 index 00000000000000..3a3065a8edb631 --- /dev/null +++ b/test/parallel/test-vm-measure-memory-multi-context.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); +const { + assertDetailedShape, + expectExperimentalWarning +} = require('../common/measure-memory'); +const vm = require('vm'); +const assert = require('assert'); + +expectExperimentalWarning(); +{ + const arr = []; + const count = 10; + for (let i = 0; i < count; ++i) { + const context = vm.createContext({ + test: new Array(100).fill('foo') + }); + arr.push(context); + } + // Check that one more context shows up in the result + vm.measureMemory({ mode: 'detailed', execution: 'eager' }) + .then(common.mustCall((result) => { + // We must hold on to the contexts here so that they + // don't get GC'ed until the measurement is complete + assert.strictEqual(arr.length, count); + assertDetailedShape(result, count); + })); +} diff --git a/test/parallel/test-vm-measure-memory.js b/test/parallel/test-vm-measure-memory.js index 7e620304e0a276..6b18db9be7a714 100644 --- a/test/parallel/test-vm-measure-memory.js +++ b/test/parallel/test-vm-measure-memory.js @@ -1,42 +1,25 @@ 'use strict'; const common = require('../common'); +const { + assertSummaryShape, + assertSingleDetailedShape, + expectExperimentalWarning +} = require('../common/measure-memory'); const assert = require('assert'); const vm = require('vm'); -common.expectWarning('ExperimentalWarning', - 'vm.measureMemory is an experimental feature. ' + - 'This feature could change at any time'); +expectExperimentalWarning(); -// The formats could change when V8 is updated, then the tests should be -// updated accordingly. -function assertSummaryShape(result) { - assert.strictEqual(typeof result, 'object'); - assert.strictEqual(typeof result.total, 'object'); - assert.strictEqual(typeof result.total.jsMemoryEstimate, 'number'); - assert(Array.isArray(result.total.jsMemoryRange)); - assert.strictEqual(typeof result.total.jsMemoryRange[0], 'number'); - assert.strictEqual(typeof result.total.jsMemoryRange[1], 'number'); -} - -function assertDetailedShape(result) { - // For now, the detailed shape is the same as the summary shape. This - // should change in future versions of V8. - return assertSummaryShape(result); -} - -// Test measuring memory of the current context +// Test eager memory measurement { - vm.measureMemory() - .then(assertSummaryShape); + vm.measureMemory({ execution: 'eager' }) + .then(common.mustCall(assertSummaryShape)); - vm.measureMemory({}) - .then(assertSummaryShape); + vm.measureMemory({ mode: 'detailed', execution: 'eager' }) + .then(common.mustCall(assertSingleDetailedShape)); - vm.measureMemory({ mode: 'summary' }) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'detailed' }) - .then(assertDetailedShape); + vm.measureMemory({ mode: 'summary', execution: 'eager' }) + .then(common.mustCall(assertSummaryShape)); assert.throws(() => vm.measureMemory(null), { code: 'ERR_INVALID_ARG_TYPE' @@ -47,24 +30,7 @@ function assertDetailedShape(result) { assert.throws(() => vm.measureMemory({ mode: 'random' }), { code: 'ERR_INVALID_ARG_VALUE' }); -} - -// Test measuring memory of the sandbox -{ - const context = vm.createContext(); - vm.measureMemory({ context }) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'summary', context },) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'detailed', context }) - .then(assertDetailedShape); - - assert.throws(() => vm.measureMemory({ mode: 'summary', context: null }), { - code: 'ERR_INVALID_ARG_TYPE' - }); - assert.throws(() => vm.measureMemory({ mode: 'summary', context: {} }), { - code: 'ERR_INVALID_ARG_TYPE' + assert.throws(() => vm.measureMemory({ execution: 'random' }), { + code: 'ERR_INVALID_ARG_VALUE' }); } From d84f1312915fe45fe0febe888db692c74894c382 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 28 Apr 2020 19:36:23 +0200 Subject: [PATCH 04/13] stream: fix stream.finished on Duplex finished would incorrectly believe that a Duplex is already closed if either the readable or writable side has completed. Fixes: https://github.com/nodejs/node/issues/33130 PR-URL: https://github.com/nodejs/node/pull/33133 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- lib/internal/streams/end-of-stream.js | 16 +++-- test/parallel/test-stream-finished.js | 86 ++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 5 deletions(-) diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js index 7d5689ddadb7fc..bc2d0ccfcaf142 100644 --- a/lib/internal/streams/end-of-stream.js +++ b/lib/internal/streams/end-of-stream.js @@ -147,10 +147,17 @@ function eos(stream, opts, callback) { if (opts.error !== false) stream.on('error', onerror); stream.on('close', onclose); - const closed = (wState && wState.closed) || (rState && rState.closed) || - (wState && wState.errorEmitted) || (rState && rState.errorEmitted) || - (wState && wState.finished) || (rState && rState.endEmitted) || - (rState && stream.req && stream.aborted); + const closed = ( + (wState && wState.closed) || + (rState && rState.closed) || + (wState && wState.errorEmitted) || + (rState && rState.errorEmitted) || + (rState && stream.req && stream.aborted) || + ( + (!writable || (wState && wState.finished)) && + (!readable || (rState && rState.endEmitted)) + ) + ); if (closed) { // TODO(ronag): Re-throw error if errorEmitted? @@ -158,6 +165,7 @@ function eos(stream, opts, callback) { // before being closed? i.e. if closed but not errored, ended or finished. // TODO(ronag): Throw some kind of error? Does it make sense // to call finished() on a "finished" stream? + // TODO(ronag): willEmitClose? process.nextTick(() => { callback(); }); diff --git a/test/parallel/test-stream-finished.js b/test/parallel/test-stream-finished.js index d4336e84db35d6..4622d2c695e1fa 100644 --- a/test/parallel/test-stream-finished.js +++ b/test/parallel/test-stream-finished.js @@ -1,7 +1,14 @@ 'use strict'; const common = require('../common'); -const { Writable, Readable, Transform, finished, Duplex } = require('stream'); +const { + Writable, + Readable, + Transform, + finished, + Duplex, + PassThrough +} = require('stream'); const assert = require('assert'); const EE = require('events'); const fs = require('fs'); @@ -396,3 +403,80 @@ testClosed((opts) => new Writable({ write() {}, ...opts })); r.destroyed = true; r.push(null); } + +{ + // Regression https://github.com/nodejs/node/issues/33130 + const response = new PassThrough(); + + class HelloWorld extends Duplex { + constructor(response) { + super({ + autoDestroy: false + }); + + this.response = response; + this.readMore = false; + + response.once('end', () => { + this.push(null); + }); + + response.on('readable', () => { + if (this.readMore) { + this._read(); + } + }); + } + + _read() { + const { response } = this; + + this.readMore = true; + + if (response.readableLength) { + this.readMore = false; + } + + let data; + while ((data = response.read()) !== null) { + this.push(data); + } + } + } + + const instance = new HelloWorld(response); + instance.setEncoding('utf8'); + instance.end(); + + (async () => { + await EE.once(instance, 'finish'); + + setImmediate(() => { + response.write('chunk 1'); + response.write('chunk 2'); + response.write('chunk 3'); + response.end(); + }); + + let res = ''; + for await (const data of instance) { + res += data; + } + + assert.strictEqual(res, 'chunk 1chunk 2chunk 3'); + })().then(common.mustCall()); +} + +{ + const p = new PassThrough(); + p.end(); + finished(p, common.mustNotCall()); +} + +{ + const p = new PassThrough(); + p.end(); + p.on('finish', common.mustCall(() => { + finished(p, common.mustNotCall()); + })); +} From e10e292c5eb82e1bbe22b597c0be0d219de5556d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 27 Apr 2020 22:15:44 +0200 Subject: [PATCH 05/13] stream: remove unused _transformState _transformState is no longer used since Transform was simplified. Refs: https://github.com/nodejs/node/pull/32763 PR-URL: https://github.com/nodejs/node/pull/33105 Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina --- lib/internal/streams/lazy_transform.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/internal/streams/lazy_transform.js b/lib/internal/streams/lazy_transform.js index 69bba428af07f5..555e6430e33588 100644 --- a/lib/internal/streams/lazy_transform.js +++ b/lib/internal/streams/lazy_transform.js @@ -59,11 +59,5 @@ ObjectDefineProperties(LazyTransform.prototype, { set: makeSetter('_writableState'), configurable: true, enumerable: true - }, - _transformState: { - get: makeGetter('_transformState'), - set: makeSetter('_transformState'), - configurable: true, - enumerable: true } }); From ab41b9808edadc034a4da60ab0f71e527d288892 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 29 Apr 2020 23:42:30 +0200 Subject: [PATCH 06/13] test: move test-process-title to sequential This test can fail when run in parallel with test-process-title-cli, which also sets the process title, which is global state on Windows. Example failure (note that `foo` does not appear in test-process-title but in test-process-title-cli): not ok 1727 parallel/test-process-title --- duration_ms: 0.156 severity: fail exitcode: 1 stack: |- assert.js:103 throw new AssertionError(obj); ^ AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: + actual - expected + 'foo' - 'd:\\a\\node\\node\\out\\Release\\node.exe' at Object. (d:\a\node\node\test\parallel\test-process-title.js:22:1) at Module._compile (internal/modules/cjs/loader.js:1176:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10) at Module.load (internal/modules/cjs/loader.js:1040:32) at Function.Module._load (internal/modules/cjs/loader.js:929:14) at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) at internal/main/run_main_module.js:17:47 { generatedMessage: true, code: 'ERR_ASSERTION', actual: 'foo', expected: 'd:\\a\\node\\node\\out\\Release\\node.exe', operator: 'strictEqual' } ... (from https://github.com/nodejs/node/runs/628144750?check_suite_focus=true) PR-URL: https://github.com/nodejs/node/pull/33150 Reviewed-By: James M Snell Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Andrey Pechkurov --- test/{parallel => sequential}/test-process-title.js | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{parallel => sequential}/test-process-title.js (100%) diff --git a/test/parallel/test-process-title.js b/test/sequential/test-process-title.js similarity index 100% rename from test/parallel/test-process-title.js rename to test/sequential/test-process-title.js From 4b2d95804b7c064ff94609f28f69098c8f6d4d31 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 28 Apr 2020 15:29:48 +0200 Subject: [PATCH 07/13] deps: V8: backport e29c62b74854 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [arraybuffer] Clean up BackingStore even if it pointer to nullptr For a zero-length BackingStore allocation, it is valid for the underlying memory to be a null pointer. However, some cleanup is still necessary, since the BackingStore may hold a reference to the allocator itself, which needs to be released when destroying the `BackingStore` instance. Change-Id: I1f168079d39e4592d2fde31fbe5f705586690e85 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2169646 Reviewed-by: Ulan Degenbaev Commit-Queue: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#67420} Refs: https://github.com/v8/v8/commit/e29c62b7485462c1ce8f4129b26e7f7a4d4b193c PR-URL: https://github.com/nodejs/node/pull/33125 Reviewed-By: Colin Ihrig Reviewed-By: Michaël Zasso Reviewed-By: Juan José Arboleda --- common.gypi | 2 +- deps/v8/src/objects/backing-store.cc | 5 ++- deps/v8/test/cctest/test-api-array-buffer.cc | 40 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index bfa9fc5cb3d650..056305368a8a1c 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.14', + 'v8_embedder_string': '-node.15', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/objects/backing-store.cc b/deps/v8/src/objects/backing-store.cc index 50e83c0ccc2470..ce138c946110b0 100644 --- a/deps/v8/src/objects/backing-store.cc +++ b/deps/v8/src/objects/backing-store.cc @@ -164,7 +164,10 @@ void BackingStore::Clear() { BackingStore::~BackingStore() { GlobalBackingStoreRegistry::Unregister(this); - if (buffer_start_ == nullptr) return; // nothing to deallocate + if (buffer_start_ == nullptr) { + Clear(); + return; + } if (is_wasm_memory_) { DCHECK(free_on_destruct_); diff --git a/deps/v8/test/cctest/test-api-array-buffer.cc b/deps/v8/test/cctest/test-api-array-buffer.cc index 3a3f2c142994e8..9ce26323d8160c 100644 --- a/deps/v8/test/cctest/test-api-array-buffer.cc +++ b/deps/v8/test/cctest/test-api-array-buffer.cc @@ -729,6 +729,46 @@ TEST(BackingStore_HoldAllocatorAlive_UntilIsolateShutdown) { CHECK(allocator_weak.expired()); } +class NullptrAllocator final : public v8::ArrayBuffer::Allocator { + public: + void* Allocate(size_t length) override { + CHECK_EQ(length, 0); + return nullptr; + } + void* AllocateUninitialized(size_t length) override { + CHECK_EQ(length, 0); + return nullptr; + } + void Free(void* data, size_t length) override { CHECK_EQ(data, nullptr); } +}; + +TEST(BackingStore_ReleaseAllocator_NullptrBackingStore) { + std::shared_ptr allocator = + std::make_shared(); + std::weak_ptr allocator_weak(allocator); + + v8::Isolate::CreateParams create_params; + create_params.array_buffer_allocator_shared = allocator; + v8::Isolate* isolate = v8::Isolate::New(create_params); + isolate->Enter(); + + allocator.reset(); + create_params.array_buffer_allocator_shared.reset(); + CHECK(!allocator_weak.expired()); + + { + std::shared_ptr backing_store = + v8::ArrayBuffer::NewBackingStore(isolate, 0); + // This should release a reference to the allocator, even though the + // buffer is empty/nullptr. + backing_store.reset(); + } + + isolate->Exit(); + isolate->Dispose(); + CHECK(allocator_weak.expired()); +} + TEST(BackingStore_HoldAllocatorAlive_AfterIsolateShutdown) { std::shared_ptr allocator = std::make_shared(); From d5e1795f5320567a91da9162558dbd95804803c8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 28 Apr 2020 18:12:43 +0200 Subject: [PATCH 08/13] test: skip memory usage tests when ASAN is enabled Running tests with an ASAN build leads to increased memory usage, rendering the memory usage assumptions in the test invalid. Refs: https://github.com/nodejs/node/pull/32776#issuecomment-620407014 PR-URL: https://github.com/nodejs/node/pull/33129 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Jiawen Geng Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matheus Marchini Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca --- test/parallel/test-crypto-dh-leak.js | 2 ++ test/sequential/test-net-bytes-per-incoming-chunk-overhead.js | 3 +++ 2 files changed, 5 insertions(+) diff --git a/test/parallel/test-crypto-dh-leak.js b/test/parallel/test-crypto-dh-leak.js index 9ba8d29e155dc6..65486dbd80cb64 100644 --- a/test/parallel/test-crypto-dh-leak.js +++ b/test/parallel/test-crypto-dh-leak.js @@ -4,6 +4,8 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +if (process.config.variables.asan) + common.skip('ASAN messes with memory measurements'); const assert = require('assert'); const crypto = require('crypto'); diff --git a/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js b/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js index 46c1f6d1a5885c..c905c22d8afa78 100644 --- a/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js +++ b/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js @@ -2,6 +2,9 @@ 'use strict'; const common = require('../common'); +if (process.config.variables.asan) + common.skip('ASAN messes with memory measurements'); + const assert = require('assert'); const net = require('net'); From 0413accc6b6e2b81784ab959b400236e4588b123 Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Tue, 28 Apr 2020 11:42:34 -0400 Subject: [PATCH 09/13] http: set default timeout in agent keepSocketAlive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous location of setting the timeout would override behaviour of custom HttpAgents' keepSocketAlive. Moving it into the default keepSocketAlive allows it to interoperate with custom agents. Fixes: https://github.com/nodejs/node/issues/33111 PR-URL: https://github.com/nodejs/node/pull/33127 Reviewed-By: Anna Henningsen Reviewed-By: Robert Nagy Reviewed-By: Luigi Pinca Reviewed-By: Juan José Arboleda Reviewed-By: Andrey Pechkurov --- lib/_http_agent.js | 10 +++--- test/parallel/test-http-agent-timeout.js | 40 ++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 2618c6c3cb8223..3db42174d73004 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -138,11 +138,6 @@ function Agent(options) { socket._httpMessage = null; this.removeSocket(socket, options); - const agentTimeout = this.options.timeout || 0; - if (socket.timeout !== agentTimeout) { - socket.setTimeout(agentTimeout); - } - socket.once('error', freeSocketErrorListener); freeSockets.push(socket); }); @@ -402,6 +397,11 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { socket.setKeepAlive(true, this.keepAliveMsecs); socket.unref(); + const agentTimeout = this.options.timeout || 0; + if (socket.timeout !== agentTimeout) { + socket.setTimeout(agentTimeout); + } + return true; }; diff --git a/test/parallel/test-http-agent-timeout.js b/test/parallel/test-http-agent-timeout.js index d8d34414d991a1..07c189e9745330 100644 --- a/test/parallel/test-http-agent-timeout.js +++ b/test/parallel/test-http-agent-timeout.js @@ -92,3 +92,43 @@ const http = require('http'); })); })); } + +{ + // Ensure custom keepSocketAlive timeout is respected + + const CUSTOM_TIMEOUT = 60; + const AGENT_TIMEOUT = 50; + + class CustomAgent extends http.Agent { + keepSocketAlive(socket) { + if (!super.keepSocketAlive(socket)) { + return false; + } + + socket.setTimeout(CUSTOM_TIMEOUT); + return true; + } + } + + const agent = new CustomAgent({ keepAlive: true, timeout: AGENT_TIMEOUT }); + + const server = http.createServer((req, res) => { + res.end(); + }); + + server.listen(0, common.mustCall(() => { + http.get({ port: server.address().port, agent }) + .on('response', common.mustCall((res) => { + const socket = res.socket; + assert(socket); + res.resume(); + socket.on('free', common.mustCall(() => { + socket.on('timeout', common.mustCall(() => { + assert.strictEqual(socket.timeout, CUSTOM_TIMEOUT); + agent.destroy(); + server.close(); + })); + })); + })); + })); +} From e9518254d79645c3c060ea8d7f4898f8e12af9bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=99=BD=E4=B8=80=E6=A2=93?= Date: Sat, 1 Feb 2020 21:36:36 +0800 Subject: [PATCH 10/13] doc: fix the spelling error in stream.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change `64kb` to `64KB` in `stream.md` PR-URL: https://github.com/nodejs/node/pull/31561 Reviewed-By: Robert Nagy Reviewed-By: Andrey Pechkurov Reviewed-By: Juan José Arboleda --- doc/api/stream.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 134687530eb95f..6229704eb91bc4 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1442,7 +1442,7 @@ If the loop terminates with a `break` or a `throw`, the stream will be destroyed. In other terms, iterating over a stream will consume the stream fully. The stream will be read in chunks of size equal to the `highWaterMark` option. In the code example above, data will be in a single chunk if the file -has less then 64kb of data because no `highWaterMark` option is provided to +has less then 64KB of data because no `highWaterMark` option is provided to [`fs.createReadStream()`][]. ### Duplex and Transform Streams @@ -1830,7 +1830,7 @@ changes: * `options` {Object} * `highWaterMark` {number} Buffer level when [`stream.write()`][stream-write] starts returning `false`. **Default:** - `16384` (16kb), or `16` for `objectMode` streams. + `16384` (16KB), or `16` for `objectMode` streams. * `decodeStrings` {boolean} Whether to encode `string`s passed to [`stream.write()`][stream-write] to `Buffer`s (with the encoding specified in the [`stream.write()`][stream-write] call) before passing @@ -2112,7 +2112,7 @@ changes: * `options` {Object} * `highWaterMark` {number} The maximum [number of bytes][hwm-gotcha] to store in the internal buffer before ceasing to read from the underlying resource. - **Default:** `16384` (16kb), or `16` for `objectMode` streams. + **Default:** `16384` (16KB), or `16` for `objectMode` streams. * `encoding` {string} If specified, then buffers will be decoded to strings using the specified encoding. **Default:** `null`. * `objectMode` {boolean} Whether this stream should behave From 26477b82a5fa1db918b4969918859d28bb0d7193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Jos=C3=A9=20Arboleda?= Date: Mon, 27 Apr 2020 14:37:10 -0500 Subject: [PATCH 11/13] doc: clarify when not to run CI on docs Collaborators won't need to run CI on documentation-only changes. PR-URL: https://github.com/nodejs/node/pull/33101 Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil Reviewed-By: Andrey Pechkurov --- doc/guides/collaborator-guide.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/guides/collaborator-guide.md b/doc/guides/collaborator-guide.md index c08834ea4a1352..128f06857b8f0a 100644 --- a/doc/guides/collaborator-guide.md +++ b/doc/guides/collaborator-guide.md @@ -177,8 +177,10 @@ All pull requests must pass continuous integration tests. Code changes must pass on [project CI server](https://ci.nodejs.org/). Pull requests that only change documentation and comments can use GitHub Actions results. -Do not land any pull requests without passing (green or yellow) CI runs. If -there are CI failures unrelated to the change in the pull request, try "Resume +Do not land any pull requests without a passing (green or yellow) CI run. +For documentation-only changes, GitHub Actions CI is sufficient. +For all other code changes, Jenkins CI must pass as well. If there are +Jenkins CI failures unrelated to the change in the pull request, try "Resume Build". It is in the left navigation of the relevant `node-test-pull-request` job. It will preserve all the green results from the current job but re-run everything else. Start a fresh CI if more than seven days have elapsed since From 7c36ec38f16add6ecfc1a3514514b55c5b610434 Mon Sep 17 00:00:00 2001 From: Kevin Locke Date: Mon, 27 Apr 2020 07:09:03 -0600 Subject: [PATCH 12/13] doc: add util.types.isArrayBufferView() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This function was added by nodejs/node#15663, but was never documented. This commit documents it. PR-URL: https://github.com/nodejs/node/pull/33092 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Gireesh Punathil Reviewed-By: Masashi Hirano Reviewed-By: Juan José Arboleda Reviewed-By: Andrey Pechkurov --- doc/api/util.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/doc/api/util.md b/doc/api/util.md index 6badb5464e42d0..95e031dc1f0a59 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1259,6 +1259,25 @@ util.types.isAnyArrayBuffer(new ArrayBuffer()); // Returns true util.types.isAnyArrayBuffer(new SharedArrayBuffer()); // Returns true ``` +### `util.types.isArrayBufferView(value)` + + +* `value` {any} +* Returns: {boolean} + +Returns `true` if the value is an instance of one of the [`ArrayBuffer`][] +views, such as typed array objects or [`DataView`][]. Equivalent to +[`ArrayBuffer.isView()`][]. + +```js +util.types.isArrayBufferView(new Int8Array()); // true +util.types.isArrayBufferView(Buffer.from('hello world')); // true +util.types.isArrayBufferView(new DataView(new ArrayBuffer(16))); // true +util.types.isArrayBufferView(new ArrayBuffer()); // false +``` + ### `util.types.isArgumentsObject(value)`