From f9dfae6480b71d8695ce4a4d872a6b6f62557e3a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 12 Mar 2020 08:18:39 +0100 Subject: [PATCH 1/3] zlib: align with streams - Ensure automatic destruction only happens after both 'end' and 'finish' has been emitted through autoDestroy. - Ensure close() callback is always invoked. - Ensure 'error' is only emitted once. --- lib/zlib.js | 29 +++++++++++++++-------------- test/parallel/test-zlib-destroy.js | 26 ++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/lib/zlib.js b/lib/zlib.js index d25b98e1ba047e..376176996e3887 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -55,6 +55,7 @@ const { } = require('internal/util/types'); const binding = internalBinding('zlib'); const assert = require('internal/assert'); +const finished = require('internal/streams/end-of-stream'); const { Buffer, kMaxLength @@ -62,6 +63,7 @@ const { const { owner_symbol } = require('internal/async_hooks').symbols; const kFlushFlag = Symbol('kFlushFlag'); +const kSyncError = Symbol('kSyncError'); const constants = internalBinding('constants').zlib; const { @@ -173,14 +175,15 @@ function zlibOnError(message, errno, code) { const self = this[owner_symbol]; // There is no way to cleanly recover. // Continuing only obscures problems. - _close(self); - self._hadError = true; // eslint-disable-next-line no-restricted-syntax const error = new Error(message); error.errno = errno; error.code = code; - self.emit('error', error); + self.destroy(error); + + // Hack for processChunkSync error. + self.emit(kSyncError, error); } // 1. Returns false for undefined and NaN @@ -260,8 +263,7 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { } } - Transform.call(this, { autoDestroy: false, ...opts }); - this._hadError = false; + Transform.call(this, { autoDestroy: true, ...opts }); this.bytesWritten = 0; this._handle = handle; handle[owner_symbol] = this; @@ -274,7 +276,6 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { this._defaultFlushFlag = flush; this._finishFlushFlag = finishFlush; this._defaultFullFlushFlag = fullFlush; - this.once('end', this.close); this._info = opts && opts.info; } ObjectSetPrototypeOf(ZlibBase.prototype, Transform.prototype); @@ -368,7 +369,7 @@ ZlibBase.prototype.flush = function(kind, callback) { }; ZlibBase.prototype.close = function(callback) { - _close(this, callback); + if (callback) finished(this, callback); this.destroy(); }; @@ -418,7 +419,10 @@ function processChunkSync(self, chunk, flushFlag) { const chunkSize = self._chunkSize; let error; - self.on('error', function onError(er) { + self.on('error', () => { + // Block unhandled exception error. + }); + self.on(kSyncError, function onError(er) { error = er; }); @@ -512,7 +516,7 @@ function processCallback() { const self = this[owner_symbol]; const state = self._writeState; - if (self._hadError || self.destroyed) { + if (self.destroyed) { this.buffer = null; this.cb(); return; @@ -578,10 +582,7 @@ function processCallback() { this.cb(); } -function _close(engine, callback) { - if (callback) - process.nextTick(callback); - +function _close(engine) { // Caller may invoke .close after a zlib error (which will null _handle). if (!engine._handle) return; @@ -678,7 +679,7 @@ ObjectSetPrototypeOf(Zlib, ZlibBase); function paramsAfterFlushCallback(level, strategy, callback) { assert(this._handle, 'zlib binding closed'); this._handle.params(level, strategy); - if (!this._hadError) { + if (!this.destroyed) { this._level = level; this._strategy = strategy; if (callback) callback(); diff --git a/test/parallel/test-zlib-destroy.js b/test/parallel/test-zlib-destroy.js index bbfce22ee2cf8a..775b7020b4ecfd 100644 --- a/test/parallel/test-zlib-destroy.js +++ b/test/parallel/test-zlib-destroy.js @@ -1,6 +1,6 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const zlib = require('zlib'); @@ -8,6 +8,24 @@ const zlib = require('zlib'); // Verify that the zlib transform does clean up // the handle when calling destroy. -const ts = zlib.createGzip(); -ts.destroy(); -assert.strictEqual(ts._handle, null); +{ + const ts = zlib.createGzip(); + ts.destroy(); + assert.strictEqual(ts._handle, null); + + ts.on('close', common.mustCall(() => { + ts.close(common.mustCall()); + })); +} + +{ + // Ensure 'error' is only emitted once. + const decompress = zlib.createGunzip(15); + + decompress.on('error', common.mustCall((err) => { + decompress.close(); + })); + + decompress.write('something invalid'); + decompress.destroy(new Error('asd')); +} From f7e7a911d446ff50ca49a7441fdd7df3e2da5636 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 13 Mar 2020 10:42:26 +0100 Subject: [PATCH 2/3] fixup: simplify? --- lib/zlib.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/zlib.js b/lib/zlib.js index 376176996e3887..cca29663171e98 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -63,7 +63,6 @@ const { const { owner_symbol } = require('internal/async_hooks').symbols; const kFlushFlag = Symbol('kFlushFlag'); -const kSyncError = Symbol('kSyncError'); const constants = internalBinding('constants').zlib; const { @@ -181,9 +180,7 @@ function zlibOnError(message, errno, code) { error.errno = errno; error.code = code; self.destroy(error); - - // Hack for processChunkSync error. - self.emit(kSyncError, error); + self._error = error; } // 1. Returns false for undefined and NaN @@ -264,6 +261,7 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { } Transform.call(this, { autoDestroy: true, ...opts }); + this._error = null; this.bytesWritten = 0; this._handle = handle; handle[owner_symbol] = this; @@ -419,10 +417,7 @@ function processChunkSync(self, chunk, flushFlag) { const chunkSize = self._chunkSize; let error; - self.on('error', () => { - // Block unhandled exception error. - }); - self.on(kSyncError, function onError(er) { + self.on('error', function onError(er) { error = er; }); @@ -436,6 +431,8 @@ function processChunkSync(self, chunk, flushFlag) { availOutBefore); // out_len if (error) throw error; + else if (self._error) + throw self._error; availOutAfter = state[0]; availInAfter = state[1]; From a98f10d2fed061dbee46aa741ca4b755ec5ec817 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 17 Mar 2020 02:16:16 +0100 Subject: [PATCH 3/3] fixup: use symbol --- lib/zlib.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/zlib.js b/lib/zlib.js index cca29663171e98..502ee61aa3bd9d 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -63,6 +63,7 @@ const { const { owner_symbol } = require('internal/async_hooks').symbols; const kFlushFlag = Symbol('kFlushFlag'); +const kError = Symbol('kError'); const constants = internalBinding('constants').zlib; const { @@ -180,7 +181,7 @@ function zlibOnError(message, errno, code) { error.errno = errno; error.code = code; self.destroy(error); - self._error = error; + self[kError] = error; } // 1. Returns false for undefined and NaN @@ -261,7 +262,7 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { } Transform.call(this, { autoDestroy: true, ...opts }); - this._error = null; + this[kError] = null; this.bytesWritten = 0; this._handle = handle; handle[owner_symbol] = this; @@ -431,8 +432,8 @@ function processChunkSync(self, chunk, flushFlag) { availOutBefore); // out_len if (error) throw error; - else if (self._error) - throw self._error; + else if (self[kError]) + throw self[kError]; availOutAfter = state[0]; availInAfter = state[1];