diff --git a/lib/_stream_transform.js b/lib/_stream_transform.js index b4fffaa98891cd..8ec87b8d7dd95b 100644 --- a/lib/_stream_transform.js +++ b/lib/_stream_transform.js @@ -67,62 +67,18 @@ const { Object } = primordials; module.exports = Transform; const { - ERR_METHOD_NOT_IMPLEMENTED, - ERR_MULTIPLE_CALLBACK, - ERR_TRANSFORM_ALREADY_TRANSFORMING, - ERR_TRANSFORM_WITH_LENGTH_0 + ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; const Duplex = require('_stream_duplex'); Object.setPrototypeOf(Transform.prototype, Duplex.prototype); Object.setPrototypeOf(Transform, Duplex); - -function afterTransform(er, data) { - const ts = this._transformState; - ts.transforming = false; - - const cb = ts.writecb; - - if (cb === null) { - return this.emit('error', new ERR_MULTIPLE_CALLBACK()); - } - - ts.writechunk = null; - ts.writecb = null; - - if (data != null) // Single equals check for both `null` and `undefined` - this.push(data); - - cb(er); - - const rs = this._readableState; - rs.reading = false; - if (rs.needReadable || rs.length < rs.highWaterMark) { - this._read(rs.highWaterMark); - } -} - - function Transform(options) { if (!(this instanceof Transform)) return new Transform(options); Duplex.call(this, options); - this._transformState = { - afterTransform: afterTransform.bind(this), - needTransform: false, - transforming: false, - writecb: null, - writechunk: null, - writeencoding: null - }; - - // We have implemented the _read method, and done the other things - // that Readable wants before the first _read call, so unset the - // sync guard flag. - this._readableState.sync = false; - if (options) { if (typeof options.transform === 'function') this._transform = options.transform; @@ -131,89 +87,53 @@ function Transform(options) { this._flush = options.flush; } - // When the writable side finishes, then flush out anything remaining. - this.on('prefinish', prefinish); -} - -function prefinish() { - if (typeof this._flush === 'function' && !this._readableState.destroyed) { - this._flush((er, data) => { - done(this, er, data); - }); - } else { - done(this, null, null); - } -} - -Transform.prototype.push = function(chunk, encoding) { - this._transformState.needTransform = false; - return Duplex.prototype.push.call(this, chunk, encoding); -}; - -// This is the part where you do stuff! -// override this function in implementation classes. -// 'chunk' is an input chunk. -// -// Call `push(newChunk)` to pass along transformed output -// to the readable side. You may call 'push' zero or more times. -// -// Call `cb(err)` when you are done with this chunk. If you pass -// an error, then that'll put the hurt on the whole operation. If you -// never call cb(), then you'll never get another chunk. -Transform.prototype._transform = function(chunk, encoding, cb) { - cb(new ERR_METHOD_NOT_IMPLEMENTED('_transform()')); + this._readableState.sync = false; + this._resume = null; }; -Transform.prototype._write = function(chunk, encoding, cb) { - const ts = this._transformState; - ts.writecb = cb; - ts.writechunk = chunk; - ts.writeencoding = encoding; - if (!ts.transforming) { - var rs = this._readableState; - if (ts.needTransform || - rs.needReadable || - rs.length < rs.highWaterMark) - this._read(rs.highWaterMark); +Transform.prototype._final = function (cb) { + if (this._flush) { + this._flush((err) => { + if (err) { + cb(err); + } else { + this.push(null); + cb(); + } + }) + } else { + this.push(null); + cb(); } }; -// Doesn't matter what the args are here. -// _transform does all the work. -// That we got here means that the readable side wants more data. -Transform.prototype._read = function(n) { - const ts = this._transformState; - - if (ts.writechunk !== null && !ts.transforming) { - ts.transforming = true; - this._transform(ts.writechunk, ts.writeencoding, ts.afterTransform); - } else { - // Mark that we need a transform, so that any data that comes in - // will get processed, now that we've asked for it. - ts.needTransform = true; +Transform.prototype._read = function (n) { + if (this._resume) { + this._resume(); + this._resume = null; } }; - -Transform.prototype._destroy = function(err, cb) { - Duplex.prototype._destroy.call(this, err, (err2) => { - cb(err2); +Transform.prototype._write = function (chunk, encoding, callback) { + this._transform.call(this, chunk, encoding, (...args) => { + if (args[0]) { + callback(args[0]); + return; + } + + if (args.length > 1) { + this.push(args[1]); + } + + const r = this._readableState; + if (r.length < r.highWaterMark || r.length === 0) { + callback(); + } else { + this._resume = callback; + } }); }; - -function done(stream, er, data) { - if (er) - return stream.emit('error', er); - - if (data != null) // Single equals check for both `null` and `undefined` - stream.push(data); - - // These two error cases are coherence checks that can likely not be tested. - if (stream._writableState.length) - throw new ERR_TRANSFORM_WITH_LENGTH_0(); - - if (stream._transformState.transforming) - throw new ERR_TRANSFORM_ALREADY_TRANSFORMING(); - return stream.push(null); -} +Transform.prototype._transform = function(chunk, encoding, cb) { + cb(new ERR_METHOD_NOT_IMPLEMENTED('_transform()')); +}; diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index c7a3047dc72268..ea2c7efcd4d27a 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -467,8 +467,10 @@ function onwrite(stream, er) { const sync = state.sync; const cb = state.writecb; - if (typeof cb !== 'function') - throw new ERR_MULTIPLE_CALLBACK(); + if (typeof cb !== 'function') { + errorOrDestroy(stream, new ERR_MULTIPLE_CALLBACK()); + return + } state.writing = false; state.writecb = null; @@ -510,7 +512,7 @@ function afterWrite(stream, state, cb) { } state.pendingcb--; cb(); - finishMaybe(stream, state); + finishMaybe(stream, state, false); } // If there's something in the buffer waiting, then process it @@ -642,24 +644,23 @@ function needFinish(state) { !state.finished && !state.writing); } -function callFinal(stream, state) { - stream._final((err) => { - state.pendingcb--; - if (err) { - errorOrDestroy(stream, err); - } else { - state.prefinished = true; - stream.emit('prefinish'); - finishMaybe(stream, state); - } - }); -} -function prefinish(stream, state) { + +function prefinish(stream, state, sync) { if (!state.prefinished && !state.finalCalled) { if (typeof stream._final === 'function' && !state.destroyed) { state.pendingcb++; state.finalCalled = true; - process.nextTick(callFinal, stream, state); + stream._final((err) => { + state.pendingcb--; + if (err) { + errorOrDestroy(stream, err, sync); + } else { + state.prefinished = true; + stream.emit('prefinish'); + finishMaybe(stream, state, sync); + } + }); + sync = false; } else { state.prefinished = true; stream.emit('prefinish'); @@ -667,30 +668,42 @@ function prefinish(stream, state) { } } -function finishMaybe(stream, state) { +function finishMaybe(stream, state, sync) { const need = needFinish(state); if (need) { - prefinish(stream, state); - if (state.pendingcb === 0) { - state.finished = true; - stream.emit('finish'); - - if (state.autoDestroy) { - // In case of duplex streams we need a way to detect - // if the readable side is ready for autoDestroy as well - const rState = stream._readableState; - if (!rState || (rState.autoDestroy && rState.endEmitted)) { - stream.destroy(); - } + prefinish(stream, state, sync); + if (state.prefinished && state.pendingcb === 0) { + if (sync) { + process.nextTick(finishWritable, stream, state); + } else { + finishWritable(stream, state); } } } return need; } +function finishWritable(stream, state) { + if (state.finished) { + return; + } + + state.finished = true; + stream.emit('finish'); + + if (state.autoDestroy) { + // In case of duplex streams we need a way to detect + // if the readable side is ready for autoDestroy as well + const rState = stream._readableState; + if (!rState || (rState.autoDestroy && rState.endEmitted)) { + stream.destroy(); + } + } +} + function endWritable(stream, state, cb) { state.ending = true; - finishMaybe(stream, state); + finishMaybe(stream, state, true); if (cb) { if (state.finished) process.nextTick(cb); diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index 0d292c8a536efb..8b61272527bb35 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -286,7 +286,8 @@ Object.setPrototypeOf(WriteStream, Writable); WriteStream.prototype._final = function(callback) { if (typeof this.fd !== 'number') { return this.once('open', function() { - this._final(callback); + // Make sure _final is called after every 'open' listener + process.nextTick(() => this._final(callback)); }); } diff --git a/lib/internal/fs/sync_write_stream.js b/lib/internal/fs/sync_write_stream.js index 522bfc829df303..dcdf26e957a581 100644 --- a/lib/internal/fs/sync_write_stream.js +++ b/lib/internal/fs/sync_write_stream.js @@ -24,14 +24,19 @@ SyncWriteStream.prototype._write = function(chunk, encoding, cb) { return true; }; -SyncWriteStream.prototype._destroy = function(err, cb) { - if (this.fd === null) // already destroy()ed - return cb(err); - - if (this.autoClose) +function close (stream) { + if (this.fd !== null && this.autoClose) closeSync(this.fd); - this.fd = null; +} + +SyncWriteStream.prototype._final = function(cb) { + close(); + cb(); +}; + +SyncWriteStream.prototype._destroy = function(err, cb) { + close(); cb(err); }; diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index ded75b255934c4..ed0853d4263129 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -108,7 +108,7 @@ function undestroy() { } } -function errorOrDestroy(stream, err) { +function errorOrDestroy(stream, err, sync) { // We have tests that rely on errors being emitted // in the same tick, so changing this is semver major. // For now when you opt-in to autoDestroy we allow @@ -124,8 +124,13 @@ function errorOrDestroy(stream, err) { if ((r && r.autoDestroy) || (w && w.autoDestroy)) stream.destroy(err); - else if (needError(stream, err)) - stream.emit('error', err); + else if (needError(stream, err)) { + if (sync) { + process.nextTick(emitErrorNT, stream, err); + } else { + stream.emit('error', err); + } + } } diff --git a/test/parallel/test-stream-pipe-flow.js b/test/parallel/test-stream-pipe-flow.js index 1f2e8f54cec409..b35e0d9d760842 100644 --- a/test/parallel/test-stream-pipe-flow.js +++ b/test/parallel/test-stream-pipe-flow.js @@ -52,10 +52,11 @@ const { Readable, Writable, PassThrough } = require('stream'); process.nextTick(() => { let data = pt.read(); if (data === null) { - pt.once('readable', () => { - data = pt.read(); - if (data !== null) wrapper.push(data); - }); + wrapper.push(data); + // pt.once('readable', () => { + // data = pt.read(); + // if (data !== null) wrapper.push(data); + // }); } else { wrapper.push(data); } diff --git a/test/parallel/test-stream-transform-constructor-set-methods.js b/test/parallel/test-stream-transform-constructor-set-methods.js index d599e768386515..c3dcf2e780681f 100644 --- a/test/parallel/test-stream-transform-constructor-set-methods.js +++ b/test/parallel/test-stream-transform-constructor-set-methods.js @@ -18,23 +18,17 @@ const _transform = common.mustCall((chunk, _, next) => { next(); }); -const _final = common.mustCall((next) => { - next(); -}); - const _flush = common.mustCall((next) => { next(); }); const t2 = new Transform({ transform: _transform, - flush: _flush, - final: _final + flush: _flush }); strictEqual(t2._transform, _transform); strictEqual(t2._flush, _flush); -strictEqual(t2._final, _final); t2.end(Buffer.from('blerg')); t2.resume(); diff --git a/test/parallel/test-stream-transform-final-sync.js b/test/parallel/test-stream-transform-final-sync.js index 1942bee1a01e8a..11890f6c297869 100644 --- a/test/parallel/test-stream-transform-final-sync.js +++ b/test/parallel/test-stream-transform-final-sync.js @@ -66,23 +66,14 @@ const t = new stream.Transform({ assert.strictEqual(++state, chunk + 2); process.nextTick(next); }, 3), - final: common.mustCall(function(done) { - state++; - // finalCallback part 1 - assert.strictEqual(state, 10); - state++; - // finalCallback part 2 - assert.strictEqual(state, 11); - done(); - }, 1), flush: common.mustCall(function(done) { state++; // fluchCallback part 1 - assert.strictEqual(state, 12); + assert.strictEqual(state, 10); process.nextTick(function() { state++; // fluchCallback part 2 - assert.strictEqual(state, 15); + assert.strictEqual(state, 11); done(); }); }, 1) @@ -90,12 +81,12 @@ const t = new stream.Transform({ t.on('finish', common.mustCall(function() { state++; // finishListener - assert.strictEqual(state, 13); + assert.strictEqual(state, 12); }, 1)); t.on('end', common.mustCall(function() { state++; // endEvent - assert.strictEqual(state, 16); + assert.strictEqual(state, 14); }, 1)); t.on('data', common.mustCall(function(d) { // dataListener @@ -106,5 +97,5 @@ t.write(4); t.end(7, common.mustCall(function() { state++; // endMethodCallback - assert.strictEqual(state, 14); + assert.strictEqual(state, 13); }, 1)); diff --git a/test/parallel/test-stream-transform-final.js b/test/parallel/test-stream-transform-final.js index 53b81cfea224e4..9a1c5a8b943bb3 100644 --- a/test/parallel/test-stream-transform-final.js +++ b/test/parallel/test-stream-transform-final.js @@ -66,25 +66,14 @@ const t = new stream.Transform({ assert.strictEqual(++state, chunk + 2); process.nextTick(next); }, 3), - final: common.mustCall(function(done) { - state++; - // finalCallback part 1 - assert.strictEqual(state, 10); - setTimeout(function() { - state++; - // finalCallback part 2 - assert.strictEqual(state, 11); - done(); - }, 100); - }, 1), flush: common.mustCall(function(done) { state++; // flushCallback part 1 - assert.strictEqual(state, 12); + assert.strictEqual(state, 10); process.nextTick(function() { state++; // flushCallback part 2 - assert.strictEqual(state, 15); + assert.strictEqual(state, 11); done(); }); }, 1) @@ -92,12 +81,12 @@ const t = new stream.Transform({ t.on('finish', common.mustCall(function() { state++; // finishListener - assert.strictEqual(state, 13); + assert.strictEqual(state, 12); }, 1)); t.on('end', common.mustCall(function() { state++; // end event - assert.strictEqual(state, 16); + assert.strictEqual(state, 14); }, 1)); t.on('data', common.mustCall(function(d) { // dataListener @@ -108,5 +97,5 @@ t.write(4); t.end(7, common.mustCall(function() { state++; // endMethodCallback - assert.strictEqual(state, 14); + assert.strictEqual(state, 13); }, 1)); diff --git a/test/parallel/test-stream-writable-finished.js b/test/parallel/test-stream-writable-finished.js index a5dfc060256a02..0f3e008df72479 100644 --- a/test/parallel/test-stream-writable-finished.js +++ b/test/parallel/test-stream-writable-finished.js @@ -28,3 +28,18 @@ const assert = require('assert'); assert.strictEqual(writable.writableFinished, true); })); } + +{ + // 'finish' must be invoked asynchronously + + const w = new Writable({ + write(chunk, enc, cb) { cb(); } + }); + + let ticked = false; + w.on('finish', common.mustCall(() => { + assert.strictEqual(ticked, true); + })); + w.end(); + ticked = true; +}