From 0b5f44a97f0341d0523eb46c0e2aa5adc1da46f4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 4 May 2016 21:18:21 +0200 Subject: [PATCH 1/2] zlib: remove `_closed` in source This is purely cleanup and carries no visible behavioural changes. Up to now, `this._closed` was used in zlib.js as a synonym of `!this._handle`. This change makes this connection explicit and removes the `_closed` property from zlib streams, as the previous duplication has been the cause of subtle errors like https://github.com/nodejs/node/issues/6034. This also makes zlib errors lead to an explicit `_close()` call rather than waiting for garbage collection to clean up the handle, thus returning memory resources earlier in the case of an error. --- lib/zlib.js | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/lib/zlib.js b/lib/zlib.js index 3915e5ddabd318..4d098d4b9cbfc2 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -364,7 +364,7 @@ function Zlib(opts, mode) { this._handle.onerror = function(message, errno) { // there is no way to cleanly recover. // continuing only obscures problems. - self._handle = null; + _close(self); self._hadError = true; var error = new Error(message); @@ -387,7 +387,6 @@ function Zlib(opts, mode) { this._buffer = Buffer.allocUnsafe(this._chunkSize); this._offset = 0; - this._closed = false; this._level = level; this._strategy = strategy; @@ -412,7 +411,7 @@ Zlib.prototype.params = function(level, strategy, callback) { if (this._level !== level || this._strategy !== strategy) { var self = this; this.flush(binding.Z_SYNC_FLUSH, function() { - assert(!self._closed, 'zlib binding closed'); + assert(self._handle, 'zlib binding closed'); self._handle.params(level, strategy); if (!self._hadError) { self._level = level; @@ -426,7 +425,7 @@ Zlib.prototype.params = function(level, strategy, callback) { }; Zlib.prototype.reset = function() { - assert(!this._closed, 'zlib binding closed'); + assert(this._handle, 'zlib binding closed'); return this._handle.reset(); }; @@ -469,15 +468,12 @@ function _close(engine, callback) { if (callback) process.nextTick(callback); - if (engine._closed) + // Caller may invoke .close after a zlib error (which will null _handle). + if (!engine._handle) return; - engine._closed = true; - - // Caller may invoke .close after a zlib error (which will null _handle). - if (engine._handle) { - engine._handle.close(); - } + engine._handle.close(); + engine._handle = null; } function emitCloseNT(self) { @@ -493,7 +489,7 @@ Zlib.prototype._transform = function(chunk, encoding, cb) { if (chunk !== null && !(chunk instanceof Buffer)) return cb(new Error('invalid input')); - if (this._closed) + if (!this._handle) return cb(new Error('zlib binding closed')); // If it's the last chunk, or a final flush, we use the Z_FINISH flush flag @@ -533,7 +529,7 @@ Zlib.prototype._processChunk = function(chunk, flushFlag, cb) { error = er; }); - assert(!this._closed, 'zlib binding closed'); + assert(this._handle, 'zlib binding closed'); do { var res = this._handle.writeSync(flushFlag, chunk, // in @@ -559,7 +555,7 @@ Zlib.prototype._processChunk = function(chunk, flushFlag, cb) { return buf; } - assert(!this._closed, 'zlib binding closed'); + assert(this._handle, 'zlib binding closed'); var req = this._handle.write(flushFlag, chunk, // in inOff, // in_off From 1796e3eabd812d5e3946abb6bae2c86f49a87693 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 4 May 2016 21:56:29 +0200 Subject: [PATCH 2/2] [squash] add `_closed` getter Add a getter for `_closed` so that the property remains accessible by legacy code. --- lib/zlib.js | 6 ++++++ test/parallel/test-zlib-close-after-error.js | 2 ++ 2 files changed, 8 insertions(+) diff --git a/lib/zlib.js b/lib/zlib.js index 4d098d4b9cbfc2..6d9f47dca2266e 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -391,6 +391,12 @@ function Zlib(opts, mode) { this._strategy = strategy; this.once('end', this.close); + + Object.defineProperty(this, '_closed', { + get: () => { return !this._handle; }, + configurable: true, + enumerable: true + }); } util.inherits(Zlib, Transform); diff --git a/test/parallel/test-zlib-close-after-error.js b/test/parallel/test-zlib-close-after-error.js index 2570ca266a6754..8e21d159337c51 100644 --- a/test/parallel/test-zlib-close-after-error.js +++ b/test/parallel/test-zlib-close-after-error.js @@ -8,7 +8,9 @@ const zlib = require('zlib'); const decompress = zlib.createGunzip(15); decompress.on('error', common.mustCall((err) => { + assert.strictEqual(decompress._closed, true); assert.doesNotThrow(() => decompress.close()); })); +assert.strictEqual(decompress._closed, false); decompress.write('something invalid');