fs: read/write streams don't emit `close` after `error`

[ronag]

[sagitsofan]

Can you please provide a code to reproduce?

[ronag]

const fs = require('fs')

fs.createReadStream('this file does not exist')
  .on('error', err => {
    console.log('error', err)
  })
  .on('close', () => {
    // Will not be called
    console.log('close')
  })

[ronag]

ping @mcollina

[mcollina]

This is definitely soemthing we would like to do.

cc @mafintosh

[Drieger]

I would like to try to fix this but I need some guidance. Is it just a matter of emitting a close event altogether with the error one? Or should some additional action be taken?

I was thinking that if a file doesn't exist, close event shouldn't be emitted once file descriptor couldn't even be opened.

[ronag]

I was thinking that if a file doesn't exist, close event shouldn't be emitted once file descriptor couldn't even be opened.

You would think that. However, the stream API (as far as I understand it) needs to emit close regardless. You can think of close as destroyed. It's just an unfortunate coincidence that files also have the concept of "open".

[ronag]

Is it just a matter of emitting a close event altogether with the error one?

I believe so.

[mcollina]

The solution to this should be based on #22795 (autoDestroy flag for streams).

[Drieger]

Ok, I'll take a look at this

[antsmartian]

@mcollina If the solution is to be based on autoDestroy then by default (when autoDestory is false) on error, close won't be called? Or you meant we need to wrap the logic of emitting the close event over here: https://github.com/nodejs/node/blob/master/lib/internal/streams/destroy.js and then incorporating the destory logic in file open methods?

[mcollina]

If autoDestroy: false  'close'  is not emitted. However by flipping autoDestroy: true  when creating the stream, then it will be emitted automatically, in the right order.

[antsmartian]

@mcollina If I understood it correctly, I did moved the code to use autoDestroy, can have a look at here. But this doesn't seems to be working (at least with the below example):

fs.createReadStream('./back12.js', {autoDestroy : true})
  .on('error', err => {
    console.log('error', err)
  })
  .on('close', () => {
    console.log('close') // doesn't get logged. 
})

I'm sure, I'm missing something here. Also, there is already a property called autoClose which is by default true which means on err, it's calling destroy.