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

[ronag]

No description provided.

[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.

[antsmartian]

@mcollina Friendly remainder 🔉

[mcollina]

@antsmartian can you please open full PR so I can do a proper review?

Some quick notes:

1. a test is missing, even if it fails it should be there. Otherwise what are you verifying it against?
2. autoDestroy: true should be passed to Readable at all times (but check autoClose)
3. getting internal/streams/destroy  should not be needed at all. Why are you doing it?

[antsmartian]

@mcollina Sure, will raise a PR. I didn't write test cases, thats the reason I didn't raise PR. Will do, once I'm done with those changes.

With respect to:

getting internal/streams/destroy should not be needed at all. Why are you doing it?

I initially thought that, internal/streams/destroy should be used as it has the mechanism to destroy the stream when autoDestroy is set to true.

[Trott]

Sure, will raise a PR. I didn't write test cases, thats the reason I didn't raise PR. Will do, once I'm done with those changes.

If you wish, you can open the PR and put [WIP] (for "work in progress") in the PR title.

[antsmartian]

@Trott Yes, but looks like my initial understanding is wrong. So I need to pick up from the above comment provided. Will try to see if I can pull out things. But other's are free to open up a PR, in the meantime :)

[ronag]

I believe this has been resolved.