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

fs.createWriteStream does not fire close event #21122

Closed
spurreiter opened this issue Jun 4, 2018 · 8 comments
Closed

fs.createWriteStream does not fire close event #21122

spurreiter opened this issue Jun 4, 2018 · 8 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@spurreiter
Copy link

  • Version: v10.3.0
  • Platform: Linux dev 4.15.0-22-generic Simple project messaging. #24-Ubuntu SMP Wed May 16 12:15:17 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem:

The following code illustrates the observed issue:

// index.js
const fs = require('fs')
const {resolve} = require('path')

function copy (source, target, cb) {
  const read = fs.createReadStream(source)
  const write = fs.createWriteStream(target, {flags: 'w'})

  function copyit () {
    if (copyit.once) return
    copyit.once = true
    console.log('copyit')
     
    write.on('error', function (err) {
      console.log('onerror')
      cb(err)
    })
    write.on('close', function () {
      console.log('onclose')
      cb(null)
    })
    read.pipe(write)
  }

  read.on('readable', copyit)
  read.on('end', copyit)
}

const source = resolve(__dirname, 'index.js')
const target = resolve(__dirname, 'copy.js')

copy(source, target, (err) => {
  console.log('end', err)
})

On node>=10.0 only the readable event fires

copyit

On node@9.11.1 the output is

copyit
onclose
end null
@DaAitch
Copy link
Contributor

DaAitch commented Jun 5, 2018

I can confirm that behaviour although I don't know if it's correct or not. Different versions have different outputs.

versions I used:

  • node-v8.9.4-linux-x64
  • node-v10.0.0-rc.2-linux-x64
  • node master (-3 days)

I've also tested it the easier way of "copying via stream" and it works as expected, both close events are emitted.

const fs = require('fs')
const {resolve} = require('path')

const source = resolve(__dirname, 'index.js')
const target = resolve(__dirname, 'copy.js')

const reader = fs.createReadStream(source)
const writer = fs.createWriteStream(target);

reader.on('close', () => {
    console.log('reader closed');
});

writer.on('close', () => {
    console.log('writer closed');
});

reader.pipe(writer);

// prints:
// reader closed
// writer closed

@ryzokuken
Copy link
Contributor

Strange. There don't seem to be any changes made in particular:

ryzokuken@flying-dutchman ~/Code/nodejs/node                                                                                        [17:58:13]
> $ glog master..v10.0.0 | grep "fs:"                                                                                             [±master ✓▴]
* 65e62e5665 fs: return stats to JS in sync methods
* 5b705cddcc fs: add 'close' event to FSWatcher
* e3579a007f fs: handle long files reading in fs.promises
* d7b162cfa0 fs: complete error message for validate function

@nodejs/fs any idea what's going on here?

@joyeecheung joyeecheung added fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. labels Jun 5, 2018
@joyeecheung
Copy link
Member

@ryzokuken This looks more like a stream issue rather than fs issue?

@ryzokuken
Copy link
Contributor

@joyeecheung it does to me as well, but the 'close' event for the WriteStream class is actually documented in the fs docs, and is inherently tied to the filesystem:

Emitted when the WriteStream's underlying file descriptor has been closed.

Therefore, I thought it'd be a filesystem bug. Will try "inspect"-ing this tomorrow, if I can.

@mcollina
Copy link
Member

This is not a bug. This behavior was changed in #18994.

@spurreiter I'm not really understading why are you deferring piping in #21122 (comment). Could you please articulate your use?

Side note, this should work (note that we are listening for the 'readable'  event only once:

// index.js
const fs = require('fs')
const {resolve} = require('path')

function copy (source, target, cb) {
  const read = fs.createReadStream(source)
  const write = fs.createWriteStream(target, {flags: 'w'})

  function copyit () {
    console.log('copyit')
     
    write.on('error', function (err) {
      console.log('onerror')
      cb(err)
    })
    write.on('close', function () {
      console.log('onclose')
      cb(null)
    })
    read.pipe(write)
  }

  read.once('readable', copyit)
  read.on('end', copyit)
}

const source = resolve(__dirname, 'index.js')
const target = resolve(__dirname, 'copy.js')

copy(source, target, (err) => {
  console.log('end', err)
})

cc @oyyd

@spurreiter
Copy link
Author

I came across this issue on trying to upgrade to node 10. The code snippet is extracted and simplified from a npm package which refuses to work with node 10 but worked well until then.
I personally prefer the code as outlined by @DaAitch.
Yes indeed using readable with once solves the problem. Thanks.

@alecgibson
Copy link

I've also run into this issue. Is it expected? Our motivation is to use the stream with a Node.js Console object, where we'd leave the stream open for the (long) lifetime of the application, during which logrotate or similar may remove the underlying file. However, we get no event in this case.

const stream = fs.createWriteStream('out.log', { flags: 'a' });
const logger = new Console(stream);

// Doesn't seem to fire?
stream.on('close', () => console.log('CLOSED');

logger.log('foo');

// This might be done by an external app for example
fs.unlinkSync('out.log');

logger.log('bar');

@sam-github
Copy link
Contributor

Unlinking a file removes its name from the parent directory, but doesn't remove the file. any open references to it remain open. You will need to periodically close and reopen the file. I wouldn't recommend building this yourself, use winston, pino, log4j or bunyan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants