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

Event 'close' emits twice (fs.createWriteStream) #31366

Closed
ledeneveugene opened this issue Jan 15, 2020 · 7 comments
Closed

Event 'close' emits twice (fs.createWriteStream) #31366

ledeneveugene opened this issue Jan 15, 2020 · 7 comments

Comments

@ledeneveugene
Copy link

if i set (emitClose: false) then on version 12.14.1 event emits once.
Documentation: By default, the stream will not emit a 'close' event after it has been destroyed. This is the opposite of the default for other Writable streams. Set the emitClose option to true to change this behavior.

const fileWriteStream = fs.createWriteStream(saveTo, {emitClose: true});
file.pipe(fileWriteStream);
fileWriteStream.on('close', function () {
console.log('fileWriteStream.on(close')
}

@himself65
Copy link
Member

const fs = require('fs')
const fileReadStream = fs.createReadStream('1.in')
const fileWriteStream = fs.createWriteStream('1.out', {
  emitClose: true
})

fileReadStream.pipe(fileWriteStream)
fileWriteStream.on('close', function () {
  console.log('fileWriteStream.on(close)')
})
fileWriteStream.on(close)
fileWriteStream.on(close)

with node v13.6.0

@himself65
Copy link
Member

more examples:

const fs = require('fs')
const fileWriteStream = fs.createWriteStream('1.out', {
  emitClose: true
})
fileWriteStream.on('close', () => {
  console.log('called')
})

fileWriteStream.write('1')
fileWriteStream.write('2')

fileWriteStream.end()

@himself65
Copy link
Member

const fs = require('fs')
const fileWriteStream = fs.createWriteStream('1.out', {
  emitClose: true
})
fileWriteStream.on('close', () => {
  console.log('called')
})

fileWriteStream.write('1')
fileWriteStream.write('2')

fileWriteStream.destroy()
events.js:298
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed
?[90m    at WriteStream._write (internal/fs/streams.js:415:33)?[39m
?[90m    at WriteStream.<anonymous> (internal/fs/streams.js:411:12)?[39m
?[90m    at Object.onceWrapper (events.js:428:26)?[39m
?[90m    at WriteStream.emit (events.js:333:22)?[39m
?[90m    at internal/fs/streams.js:402:12?[39m
?[90m    at FSReqCallback.oncomplete (fs.js:154:23)?[39m
Emitted 'error' event on WriteStream instance at:
?[90m    at errorOrDestroy (internal/streams/destroy.js:128:12)?[39m
?[90m    at onwriteError (_stream_writable.js:462:3)?[39m
?[90m    at onwrite (_stream_writable.js:483:7)?[39m
?[90m    at WriteStream._write (internal/fs/streams.js:415:30)?[39m
?[90m    at WriteStream.<anonymous> (internal/fs/streams.js:411:12)?[39m
    [... lines matching original stack trace ...]
?[90m    at FSReqCallback.oncomplete (fs.js:154:23)?[39m {
  code: ?[32m'ERR_STREAM_DESTROYED'?[39m
}

@himself65
Copy link
Member

I think these problems have fixed on 67ed526.

And that commit hasn't published to the node.js version

related PR: #30851

@ledeneveugene
Copy link
Author

I think these problems have fixed on [67ed526]

Should i close issue or developers do it?

@ronag
Copy link
Member

ronag commented Jan 16, 2020

const fileWriteStream = fs.createWriteStream('1.out', {
  emitClose: true
})

Don't use emitClose: true here. It will emit 'close' anyway. This is an internal option that is only intended for stream implementations.

Preferrably it shouldn't even be forwarded.

ronag added a commit to nxtedition/node that referenced this issue Jan 16, 2020
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

Fixes: nodejs#31366
@ledeneveugene
Copy link
Author

ledeneveugene commented Jan 17, 2020

Don't use emitClose: true here. It will emit 'close' anyway. This is an internal option that is only intended for stream implementations.
Preferrably it shouldn't even be forwarded.

I tried without emitClose: true on node versions 10.16.3 and 12.14.1. You are rigth. Thank you!

Trott pushed a commit that referenced this issue Jan 20, 2020
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this issue Feb 17, 2020
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants