From b0b52b2023f5cd0df4ae921850815586b4313dca Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 18 Jul 2020 08:51:53 -0700 Subject: [PATCH 1/8] test: fix flaky test-watch-file PR-URL: https://github.com/nodejs/node/pull/34420 Reviewed-By: Anna Henningsen Reviewed-By: Myles Borins --- test/pummel/test-watch-file.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/test/pummel/test-watch-file.js b/test/pummel/test-watch-file.js index 9bac9004835c59..bbbbf396d72227 100644 --- a/test/pummel/test-watch-file.js +++ b/test/pummel/test-watch-file.js @@ -34,8 +34,11 @@ fs.closeSync(fs.openSync(f, 'w')); let changes = 0; function watchFile() { fs.watchFile(f, (curr, prev) => { + // Make sure there is at least one watch event that shows a changed mtime. + if (curr.mtime <= prev.mtime) { + return; + } changes++; - assert.notDeepStrictEqual(curr.mtime, prev.mtime); fs.unwatchFile(f); watchFile(); fs.unwatchFile(f); @@ -44,10 +47,17 @@ function watchFile() { watchFile(); +function changeFile() { + const fd = fs.openSync(f, 'w+'); + fs.writeSync(fd, 'xyz\n'); + fs.closeSync(fd); +} -const fd = fs.openSync(f, 'w+'); -fs.writeSync(fd, 'xyz\n'); -fs.closeSync(fd); +changeFile(); +const interval = setInterval(changeFile, 1000); +// Use unref() here so fs.watchFile() watcher is the only thing keeping the +// event loop open. +interval.unref(); process.on('exit', function() { assert.ok(changes > 0); From f8eaeb0c8e3196eb9d2ec8fd818c36aa27e31f72 Mon Sep 17 00:00:00 2001 From: Andrey Pechkurov Date: Tue, 23 Jun 2020 17:18:31 +0300 Subject: [PATCH 2/8] zlib: switch to lazy init for zlib streams PR-URL: https://github.com/nodejs/node/pull/34048 Reviewed-By: Anna Henningsen --- lib/zlib.js | 22 ++++--- src/node_zlib.cc | 59 ++++++++++++++----- test/parallel/test-zlib-reset-before-write.js | 37 ++++++++++++ 3 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-zlib-reset-before-write.js diff --git a/lib/zlib.js b/lib/zlib.js index fc8e378f41feb5..cf571f8a0ba901 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -664,18 +664,13 @@ function Zlib(opts, mode) { // to come up with a good solution that doesn't break our internal API, // and with it all supported npm versions at the time of writing. this._writeState = new Uint32Array(2); - if (!handle.init(windowBits, - level, - memLevel, - strategy, - this._writeState, - processCallback, - dictionary)) { - // TODO(addaleax): Sometimes we generate better error codes in C++ land, - // e.g. ERR_BROTLI_PARAM_SET_FAILED -- it's hard to access them with - // the current bindings setup, though. - throw new ERR_ZLIB_INITIALIZATION_FAILED(); - } + handle.init(windowBits, + level, + memLevel, + strategy, + this._writeState, + processCallback, + dictionary); ZlibBase.call(this, opts, mode, handle, zlibDefaultOpts); @@ -821,6 +816,9 @@ function Brotli(opts, mode) { new binding.BrotliDecoder(mode) : new binding.BrotliEncoder(mode); this._writeState = new Uint32Array(2); + // TODO(addaleax): Sometimes we generate better error codes in C++ land, + // e.g. ERR_BROTLI_PARAM_SET_FAILED -- it's hard to access them with + // the current bindings setup, though. if (!handle.init(brotliInitParamsArray, this._writeState, processCallback)) { diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 85e87327c07797..efb11debf8f40d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -139,8 +139,8 @@ class ZlibContext : public MemoryRetainer { CompressionError ResetStream(); // Zlib-specific: - CompressionError Init(int level, int window_bits, int mem_level, int strategy, - std::vector&& dictionary); + void Init(int level, int window_bits, int mem_level, int strategy, + std::vector&& dictionary); void SetAllocationFunctions(alloc_func alloc, free_func free, void* opaque); CompressionError SetParams(int level, int strategy); @@ -157,7 +157,10 @@ class ZlibContext : public MemoryRetainer { private: CompressionError ErrorForMessage(const char* message) const; CompressionError SetDictionary(); + bool InitZlib(); + Mutex mutex_; // Protects zlib_init_done_. + bool zlib_init_done_ = false; int err_ = 0; int flush_ = 0; int level_ = 0; @@ -615,13 +618,8 @@ class ZlibStream : public CompressionStream { AllocScope alloc_scope(wrap); wrap->context()->SetAllocationFunctions( AllocForZlib, FreeForZlib, static_cast(wrap)); - const CompressionError err = - wrap->context()->Init(level, window_bits, mem_level, strategy, - std::move(dictionary)); - if (err.IsError()) - wrap->EmitError(err); - - return args.GetReturnValue().Set(!err.IsError()); + wrap->context()->Init(level, window_bits, mem_level, strategy, + std::move(dictionary)); } static void Params(const FunctionCallbackInfo& args) { @@ -724,6 +722,15 @@ using BrotliEncoderStream = BrotliCompressionStream; using BrotliDecoderStream = BrotliCompressionStream; void ZlibContext::Close() { + { + Mutex::ScopedLock lock(mutex_); + if (!zlib_init_done_) { + dictionary_.clear(); + mode_ = NONE; + return; + } + } + CHECK_LE(mode_, UNZIP); int status = Z_OK; @@ -742,6 +749,11 @@ void ZlibContext::Close() { void ZlibContext::DoThreadPoolWork() { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return; + } + const Bytef* next_expected_header_byte = nullptr; // If the avail_out is left at 0, then it means that it ran out @@ -897,6 +909,11 @@ CompressionError ZlibContext::GetErrorInfo() const { CompressionError ZlibContext::ResetStream() { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return ErrorForMessage("Failed to init stream before reset"); + } + err_ = Z_OK; switch (mode_) { @@ -930,7 +947,7 @@ void ZlibContext::SetAllocationFunctions(alloc_func alloc, } -CompressionError ZlibContext::Init( +void ZlibContext::Init( int level, int window_bits, int mem_level, int strategy, std::vector&& dictionary) { if (!((window_bits == 0) && @@ -974,6 +991,15 @@ CompressionError ZlibContext::Init( window_bits_ *= -1; } + dictionary_ = std::move(dictionary); +} + +bool ZlibContext::InitZlib() { + Mutex::ScopedLock lock(mutex_); + if (zlib_init_done_) { + return false; + } + switch (mode_) { case DEFLATE: case GZIP: @@ -995,15 +1021,15 @@ CompressionError ZlibContext::Init( UNREACHABLE(); } - dictionary_ = std::move(dictionary); - if (err_ != Z_OK) { dictionary_.clear(); mode_ = NONE; - return ErrorForMessage("zlib error"); + return true; } - return SetDictionary(); + SetDictionary(); + zlib_init_done_ = true; + return true; } @@ -1040,6 +1066,11 @@ CompressionError ZlibContext::SetDictionary() { CompressionError ZlibContext::SetParams(int level, int strategy) { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return ErrorForMessage("Failed to init stream before set parameters"); + } + err_ = Z_OK; switch (mode_) { diff --git a/test/parallel/test-zlib-reset-before-write.js b/test/parallel/test-zlib-reset-before-write.js new file mode 100644 index 00000000000000..6b2b107d25d313 --- /dev/null +++ b/test/parallel/test-zlib-reset-before-write.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const zlib = require('zlib'); + +// Tests that zlib streams support .reset() and .params() +// before the first write. That is important to ensure that +// lazy init of zlib native library handles these cases. + +for (const fn of [ + (z, cb) => { + z.reset(); + cb(); + }, + (z, cb) => z.params(0, zlib.constants.Z_DEFAULT_STRATEGY, cb) +]) { + const deflate = zlib.createDeflate(); + const inflate = zlib.createInflate(); + + deflate.pipe(inflate); + + const output = []; + inflate + .on('error', (err) => { + assert.ifError(err); + }) + .on('data', (chunk) => output.push(chunk)) + .on('end', common.mustCall( + () => assert.deepStrictEqual(Buffer.concat(output).toString(), 'abc'))); + + fn(deflate, () => { + fn(inflate, () => { + deflate.write('abc'); + deflate.end(); + }); + }); +} From 07b3aae04e659d3681b9055164f4543c8c59b07a Mon Sep 17 00:00:00 2001 From: Harshitha K P Date: Sat, 18 Jul 2020 13:01:44 +0530 Subject: [PATCH 3/8] doc: add HarshithaKP to collaborators Fixes: https://github.com/nodejs/node/issues/34242 PR-URL: https://github.com/nodejs/node/pull/34417 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil Reviewed-By: Trivikram Kamat Reviewed-By: Yongsheng Zhang --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 35f8b4d84ff9cc..9cc3dd2c82ea3d 100644 --- a/README.md +++ b/README.md @@ -311,6 +311,8 @@ For information about the governance of the Node.js project, see **Gireesh Punathil** <gpunathi@in.ibm.com> (he/him) * [guybedford](https://github.com/guybedford) - **Guy Bedford** <guybedford@gmail.com> (he/him) +* [HarshithaKP](https://github.com/HarshithaKP) - +**Harshitha K P** <harshitha014@gmail.com> (she/her) * [hashseed](https://github.com/hashseed) - **Yang Guo** <yangguo@chromium.org> (he/him) * [himself65](https://github.com/himself65) - From f4f191bbc26c367ed8fa56c2d1297ef437c5f0fb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 21 Jul 2020 08:56:47 +0800 Subject: [PATCH 4/8] build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot Otherwise the build would fail with `./configure --experimental-quic --ninja` as the list of per-Environment values would not match and the code cache builder would not generate code cache for the quic JS sources. This is more or less a band-aid - a proper fix would be to aggregate these flags into something that can be included by all these different binary targets. See https://github.com/nodejs/node/issues/31074. PR-URL: https://github.com/nodejs/node/pull/34454 Fixes: https://github.com/nodejs/node/issues/34435 Reviewed-By: James M Snell Reviewed-By: Jiawen Geng --- node.gyp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/node.gyp b/node.gyp index acaf2cef8fb10a..497c06c6bc0a6c 100644 --- a/node.gyp +++ b/node.gyp @@ -1376,6 +1376,11 @@ 'HAVE_OPENSSL=1', ], }], + [ 'node_use_openssl=="true" and experimental_quic==1', { + 'defines': [ + 'NODE_EXPERIMENTAL_QUIC=1', + ], + }], ['v8_enable_inspector==1', { 'defines': [ 'HAVE_INSPECTOR=1', @@ -1430,6 +1435,11 @@ 'HAVE_OPENSSL=1', ], }], + [ 'node_use_openssl=="true" and experimental_quic==1', { + 'defines': [ + 'NODE_EXPERIMENTAL_QUIC=1', + ], + }], ['v8_enable_inspector==1', { 'defines': [ 'HAVE_INSPECTOR=1', From 7603c7e50c3ae453db9702916f740618029020ba Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 16 Jul 2020 14:38:21 +0200 Subject: [PATCH 5/8] worker: set trackUnmanagedFds to true by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents accidental resource leaks when terminating or exiting Worker that own FDs opened through `fs.open()`. Refs: https://github.com/nodejs/node/pull/34303 PR-URL: https://github.com/nodejs/node/pull/34394 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Rich Trott Reviewed-By: Tobias Nießen Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- doc/api/worker_threads.md | 5 ++++- lib/internal/worker.js | 2 +- test/parallel/test-worker-track-unmanaged-fds.js | 15 ++++++++++++++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 7162225c8b1867..db00f7e713c8e0 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -621,6 +621,9 @@ if (isMainThread) {