Commit a940143
committed
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.
PR-URL: #32220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>1 parent 543c046 commit a940143
2 files changed
Lines changed: 34 additions & 17 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -55,13 +55,15 @@ const { | |||
| 55 | 55 | } = require('internal/util/types'); | |
| 56 | 56 | const binding = internalBinding('zlib'); | |
| 57 | 57 | const assert = require('internal/assert'); | |
| 58 | + const finished = require('internal/streams/end-of-stream'); | ||
| 58 | 59 | const { | |
| 59 | 60 | Buffer, | |
| 60 | 61 | kMaxLength | |
| 61 | 62 | } = require('buffer'); | |
| 62 | 63 | const { owner_symbol } = require('internal/async_hooks').symbols; | |
| 63 | 64 | ||
| 64 | 65 | const kFlushFlag = Symbol('kFlushFlag'); | |
| 66 | + const kError = Symbol('kError'); | ||
| 65 | 67 | ||
| 66 | 68 | const constants = internalBinding('constants').zlib; | |
| 67 | 69 | const { | |
@@ -173,14 +175,13 @@ function zlibOnError(message, errno, code) { | |||
| 173 | 175 | const self = this[owner_symbol]; | |
| 174 | 176 | // There is no way to cleanly recover. | |
| 175 | 177 | // Continuing only obscures problems. | |
| 176 | - _close(self); | ||
| 177 | - self._hadError = true; | ||
| 178 | 178 | ||
| 179 | 179 | // eslint-disable-next-line no-restricted-syntax | |
| 180 | 180 | const error = new Error(message); | |
| 181 | 181 | error.errno = errno; | |
| 182 | 182 | error.code = code; | |
| 183 | - self.emit('error', error); | ||
| 183 | + self.destroy(error); | ||
| 184 | + self[kError] = error; | ||
| 184 | 185 | } | |
| 185 | 186 | ||
| 186 | 187 | // 1. Returns false for undefined and NaN | |
@@ -260,8 +261,8 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { | |||
| 260 | 261 | } | |
| 261 | 262 | } | |
| 262 | 263 | ||
| 263 | - Transform.call(this, { autoDestroy: false, ...opts }); | ||
| 264 | - this._hadError = false; | ||
| 264 | + Transform.call(this, { autoDestroy: true, ...opts }); | ||
| 265 | + this[kError] = null; | ||
| 265 | 266 | this.bytesWritten = 0; | |
| 266 | 267 | this._handle = handle; | |
| 267 | 268 | handle[owner_symbol] = this; | |
@@ -274,7 +275,6 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) { | |||
| 274 | 275 | this._defaultFlushFlag = flush; | |
| 275 | 276 | this._finishFlushFlag = finishFlush; | |
| 276 | 277 | this._defaultFullFlushFlag = fullFlush; | |
| 277 | - this.once('end', this.close); | ||
| 278 | 278 | this._info = opts && opts.info; | |
| 279 | 279 | } | |
| 280 | 280 | ObjectSetPrototypeOf(ZlibBase.prototype, Transform.prototype); | |
@@ -368,7 +368,7 @@ ZlibBase.prototype.flush = function(kind, callback) { | |||
| 368 | 368 | }; | |
| 369 | 369 | ||
| 370 | 370 | ZlibBase.prototype.close = function(callback) { | |
| 371 | - _close(this, callback); | ||
| 371 | + if (callback) finished(this, callback); | ||
| 372 | 372 | this.destroy(); | |
| 373 | 373 | }; | |
| 374 | 374 | ||
@@ -432,6 +432,8 @@ function processChunkSync(self, chunk, flushFlag) { | |||
| 432 | 432 | availOutBefore); // out_len | |
| 433 | 433 | if (error) | |
| 434 | 434 | throw error; | |
| 435 | + else if (self[kError]) | ||
| 436 | + throw self[kError]; | ||
| 435 | 437 | ||
| 436 | 438 | availOutAfter = state[0]; | |
| 437 | 439 | availInAfter = state[1]; | |
@@ -512,7 +514,7 @@ function processCallback() { | |||
| 512 | 514 | const self = this[owner_symbol]; | |
| 513 | 515 | const state = self._writeState; | |
| 514 | 516 | ||
| 515 | - if (self._hadError || self.destroyed) { | ||
| 517 | + if (self.destroyed) { | ||
| 516 | 518 | this.buffer = null; | |
| 517 | 519 | this.cb(); | |
| 518 | 520 | return; | |
@@ -578,10 +580,7 @@ function processCallback() { | |||
| 578 | 580 | this.cb(); | |
| 579 | 581 | } | |
| 580 | 582 | ||
| 581 | - function _close(engine, callback) { | ||
| 582 | - if (callback) | ||
| 583 | - process.nextTick(callback); | ||
| 584 | - | ||
| 583 | + function _close(engine) { | ||
| 585 | 584 | // Caller may invoke .close after a zlib error (which will null _handle). | |
| 586 | 585 | if (!engine._handle) | |
| 587 | 586 | return; | |
@@ -678,7 +677,7 @@ ObjectSetPrototypeOf(Zlib, ZlibBase); | |||
| 678 | 677 | function paramsAfterFlushCallback(level, strategy, callback) { | |
| 679 | 678 | assert(this._handle, 'zlib binding closed'); | |
| 680 | 679 | this._handle.params(level, strategy); | |
| 681 | - if (!this._hadError) { | ||
| 680 | + if (!this.destroyed) { | ||
| 682 | 681 | this._level = level; | |
| 683 | 682 | this._strategy = strategy; | |
| 684 | 683 | if (callback) callback(); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,13 +1,31 @@ | |||
| 1 | 1 | 'use strict'; | |
| 2 | 2 | ||
| 3 | - require('../common'); | ||
| 3 | + const common = require('../common'); | ||
| 4 | 4 | ||
| 5 | 5 | const assert = require('assert'); | |
| 6 | 6 | const zlib = require('zlib'); | |
| 7 | 7 | ||
| 8 | 8 | // Verify that the zlib transform does clean up | |
| 9 | 9 | // the handle when calling destroy. | |
| 10 | 10 | ||
| 11 | - const ts = zlib.createGzip(); | ||
| 12 | - ts.destroy(); | ||
| 13 | - assert.strictEqual(ts._handle, null); | ||
| 11 | + { | ||
| 12 | + const ts = zlib.createGzip(); | ||
| 13 | + ts.destroy(); | ||
| 14 | + assert.strictEqual(ts._handle, null); | ||
| 15 | + | ||
| 16 | + ts.on('close', common.mustCall(() => { | ||
| 17 | + ts.close(common.mustCall()); | ||
| 18 | + })); | ||
| 19 | + } | ||
| 20 | + | ||
| 21 | + { | ||
| 22 | + // Ensure 'error' is only emitted once. | ||
| 23 | + const decompress = zlib.createGunzip(15); | ||
| 24 | + | ||
| 25 | + decompress.on('error', common.mustCall((err) => { | ||
| 26 | + decompress.close(); | ||
| 27 | + })); | ||
| 28 | + | ||
| 29 | + decompress.write('something invalid'); | ||
| 30 | + decompress.destroy(new Error('asd')); | ||
| 31 | + } | ||
0 commit comments