Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent stream events using destroy method #25373

Closed
greguz opened this issue Jan 7, 2019 · 3 comments
Closed

Inconsistent stream events using destroy method #25373

greguz opened this issue Jan 7, 2019 · 3 comments
Labels
question Issues that look for answers. stream Issues and PRs related to the stream subsystem.

Comments

@greguz
Copy link
Contributor

greguz commented Jan 7, 2019

  • Version: v8.15.0
  • Platform: Linux 4.20.0-arch1-1-ARCH
  • Subsystem: stream

As for official Node.js docs, with a custom Writable, the only standard way to know if the stream has finished, is to listen the finish event.

const { Writable } = require('stream')

const stream = new Writable({
  write(chunk, encoding, callback) {
    callback()
  },
})

stream
  .on('close', () => console.log('close'))
  .on('error', () => console.log('error'))
  .on('finish', () => console.log('finish'))

stream.write('test')
stream.end()

The code above works correctly on both node 8 and 10, with the finish event emitted.

const { Writable } = require('stream')

const stream = new Writable({
  write(chunk, encoding, callback) {
    callback()
  },
})

stream
  .on('close', () => console.log('close'))
  .on('error', () => console.log('error'))
  .on('finish', () => console.log('finish'))

stream.destroy(new Error())

// node v8.15.0
// - finish
// - error
//
// node v10.15.0
// - error
// - close

The problem I've noticed is during the usage of the destroy() method, on node 8 the stream will emit first the finish event, then a error event, and no close event.

On node 10, the finish event is not emitted.

I'm not sure if the problem is node 8 or 10, but by reading the docs I suppose the finish event have to be emitted after a possible error.

@bnoordhuis bnoordhuis added question Issues that look for answers. stream Issues and PRs related to the stream subsystem. labels Jan 8, 2019
@bnoordhuis
Copy link
Member

cc @nodejs/streams

@mcollina
Copy link
Member

mcollina commented Jan 9, 2019

'close' is always emitted both during normal or error shutdown. This behavior has been added in #18438. Also #19836 is relevant. Both of those are semver-major changes, and the behavior in Node 10 simplifies event management and error handling. If you would like to have the same behavior on Node 8, you can use the http://npm.im/readable-stream module.

This sentence is in the docs:

Not all Writable streams will emit the 'close' event.

needs to be updated as all streams will emit 'close' (because of the changes in the above PRs) if the emitClose: true option is passed in. This is true  by default.

As for official Node.js docs, with a custom Writable, the only standard way to know if the stream has finished, is to listen the finish event.

That is not true, as the docs report:

The 'finish' event is emitted after the stream.end() method has been called, and all data has been flushed to the underlying system.

It does not mention anything related to the error conditions. I'm not 100% sure why 'finish' is emitted on Node 8 when an 'error' happens, and it's likely a bug (which we should likely not fix, because 8 has been around for so long).

@greguz
Copy link
Contributor Author

greguz commented Jan 9, 2019

Thank you for the response.

Just for info, I've noticed the same behavior with Readable streams. I've also found a workaround, a setImmediate (nextTick does not work) on end, finish and close event callbacks does the trick.

@greguz greguz closed this as completed Jan 9, 2019
mcollina added a commit to mcollina/node that referenced this issue Jan 10, 2019
mcollina added a commit that referenced this issue Jan 12, 2019
See: #25373
See: #18438

PR-URL: #25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
addaleax pushed a commit that referenced this issue Jan 14, 2019
See: #25373
See: #18438

PR-URL: #25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
See: nodejs#25373
See: nodejs#18438

PR-URL: nodejs#25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 28, 2019
See: #25373
See: #18438

PR-URL: #25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
BethGriggs pushed a commit that referenced this issue May 10, 2019
See: #25373
See: #18438

PR-URL: #25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this issue May 16, 2019
See: #25373
See: #18438

PR-URL: #25413
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants