Skip to content

Commit

Permalink
zlib: remove _closed in source
Browse files Browse the repository at this point in the history
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 #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.

Add a getter for `_closed` so that the property remains accessible
by legacy code.

PR-URL: #6574
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
addaleax committed May 17, 2016
1 parent ba10ea8 commit b53473f
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 14 deletions.
30 changes: 16 additions & 14 deletions lib/zlib.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -387,11 +387,16 @@ function Zlib(opts, mode) {

this._buffer = Buffer.allocUnsafe(this._chunkSize);
this._offset = 0;
this._closed = false;
this._level = level;
this._strategy = strategy;

this.once('end', this.close);

Object.defineProperty(this, '_closed', {
get: () => { return !this._handle; },
configurable: true,
enumerable: true
});
}

util.inherits(Zlib, Transform);
Expand All @@ -412,7 +417,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;
Expand All @@ -426,7 +431,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();
};

Expand Down Expand Up @@ -469,15 +474,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) {
Expand All @@ -493,7 +495,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
Expand Down Expand Up @@ -533,7 +535,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
Expand All @@ -559,7 +561,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
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-zlib-close-after-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

0 comments on commit b53473f

Please sign in to comment.