Skip to content

Commit 6441556

Browse files
addaleaxrvagg
authored andcommitted
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 #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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 5a1e823 commit 6441556

File tree

2 files changed

+18
-14
lines changed

2 files changed

+18
-14
lines changed

lib/zlib.js

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ function Zlib(opts, mode) {
364364
this._handle.onerror = function(message, errno) {
365365
// there is no way to cleanly recover.
366366
// continuing only obscures problems.
367-
self._handle = null;
367+
_close(self);
368368
self._hadError = true;
369369

370370
var error = new Error(message);
@@ -387,11 +387,16 @@ function Zlib(opts, mode) {
387387

388388
this._buffer = Buffer.allocUnsafe(this._chunkSize);
389389
this._offset = 0;
390-
this._closed = false;
391390
this._level = level;
392391
this._strategy = strategy;
393392

394393
this.once('end', this.close);
394+
395+
Object.defineProperty(this, '_closed', {
396+
get: () => { return !this._handle; },
397+
configurable: true,
398+
enumerable: true
399+
});
395400
}
396401

397402
util.inherits(Zlib, Transform);
@@ -412,7 +417,7 @@ Zlib.prototype.params = function(level, strategy, callback) {
412417
if (this._level !== level || this._strategy !== strategy) {
413418
var self = this;
414419
this.flush(binding.Z_SYNC_FLUSH, function() {
415-
assert(!self._closed, 'zlib binding closed');
420+
assert(self._handle, 'zlib binding closed');
416421
self._handle.params(level, strategy);
417422
if (!self._hadError) {
418423
self._level = level;
@@ -426,7 +431,7 @@ Zlib.prototype.params = function(level, strategy, callback) {
426431
};
427432

428433
Zlib.prototype.reset = function() {
429-
assert(!this._closed, 'zlib binding closed');
434+
assert(this._handle, 'zlib binding closed');
430435
return this._handle.reset();
431436
};
432437

@@ -469,15 +474,12 @@ function _close(engine, callback) {
469474
if (callback)
470475
process.nextTick(callback);
471476

472-
if (engine._closed)
477+
// Caller may invoke .close after a zlib error (which will null _handle).
478+
if (!engine._handle)
473479
return;
474480

475-
engine._closed = true;
476-
477-
// Caller may invoke .close after a zlib error (which will null _handle).
478-
if (engine._handle) {
479-
engine._handle.close();
480-
}
481+
engine._handle.close();
482+
engine._handle = null;
481483
}
482484

483485
function emitCloseNT(self) {
@@ -493,7 +495,7 @@ Zlib.prototype._transform = function(chunk, encoding, cb) {
493495
if (chunk !== null && !(chunk instanceof Buffer))
494496
return cb(new Error('invalid input'));
495497

496-
if (this._closed)
498+
if (!this._handle)
497499
return cb(new Error('zlib binding closed'));
498500

499501
// If it's the last chunk, or a final flush, we use the Z_FINISH flush flag
@@ -533,7 +535,7 @@ Zlib.prototype._processChunk = function(chunk, flushFlag, cb) {
533535
error = er;
534536
});
535537

536-
assert(!this._closed, 'zlib binding closed');
538+
assert(this._handle, 'zlib binding closed');
537539
do {
538540
var res = this._handle.writeSync(flushFlag,
539541
chunk, // in
@@ -559,7 +561,7 @@ Zlib.prototype._processChunk = function(chunk, flushFlag, cb) {
559561
return buf;
560562
}
561563

562-
assert(!this._closed, 'zlib binding closed');
564+
assert(this._handle, 'zlib binding closed');
563565
var req = this._handle.write(flushFlag,
564566
chunk, // in
565567
inOff, // in_off

test/parallel/test-zlib-close-after-error.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ const zlib = require('zlib');
88
const decompress = zlib.createGunzip(15);
99

1010
decompress.on('error', common.mustCall((err) => {
11+
assert.strictEqual(decompress._closed, true);
1112
assert.doesNotThrow(() => decompress.close());
1213
}));
1314

15+
assert.strictEqual(decompress._closed, false);
1416
decompress.write('something invalid');

0 commit comments

Comments
 (0)