"WriteStream.close" from "fs" module not executing the callback

[gkubisa]

The callback passed to WriteStream.close function from the "fs" module is not called in certain situations (see below). The same applies to the ReadStream.

var fs = require('fs');
var a = fs.createWriteStream('/tmp/aaa');

a.close(console.log.bind(null, 'closed'));
// prints "closed"

a.close(console.log.bind(null, 'closed'));
// the "close" callback is not executed the second time

var fs = require('fs');
var a = fs.createWriteStream('/dev/full');

a.on('error', console.log.bind(null, 'error'));
a.write('a');
// prints "error"

a.close(console.log.bind(null, 'closed'));
// the "close" callback is not called after an error

[bnoordhuis]

The first example is basically a logic bug because .close() is not an idempotent operation. We could probably do a little better there because node does attempt to close the file descriptor again. That will fail with EBADF most of the time but sometimes, it ends up closing the wrong file descriptor.

Your second example works for me with v4.1.0.

[bnoordhuis]

#2955

[gkubisa]

Sorry, I tested my original examples in REPL. The snippets below can be executed as scripts.

In Scripts 1, 2 & 3, the file descriptor is already closed, when "close" function is called.
Result: callback is NOT called.
Expected: callback should be called, possibly with error.

In Script 4, the "closed" property is not set and "close" event has not been emitted yet, when "close" function is called.
Result: callback is called and "error" (EBADF) is emitted.
Expected: error should not be emitted.

Script 1.

// prints:
// "closed1"
var fs = require('fs');
var a = fs.createWriteStream('/tmp/aaa');

a.close(function () {
    console.log('closed1');
    a.close(console.log.bind(null, 'closed2'));
});

Script 2.

// prints:
// "error { [Error: ENOSPC: no space left on device, write] errno: -28, code: 'ENOSPC', syscall: 'write' }"
// "closed1"
var fs = require('fs');
var a = fs.createWriteStream('/dev/full');

a.on('close', function () {
    console.log('closed1');
    a.close(console.log.bind(null, 'closed2'));
});
a.on('error', console.log.bind(null, 'error'));
a.write('a');

Script 3.

// prints:
// "closed1"
var fs = require('fs');
var a = fs.createWriteStream('/tmp/aaa');

a.on('close', function () {
    console.log('closed1');
    a.close(console.log.bind(null, 'closed2'));
});
a.end();

Script 4.

// prints:
// "finish closed=undefined"
// "closed1"
// "closed2"
// "error { [Error: EBADF: bad file descriptor, close] errno: -9, code: 'EBADF', syscall: 'close' }"
var fs = require('fs');
var a = fs.createWriteStream('/tmp/aaa');

a.on('error', console.log.bind(null, 'error'));
a.on('close', console.log.bind(null, 'closed1'));
a.on('finish', function () {
    console.log('finish', 'closed=' + a.closed);
    a.close(console.log.bind(null, 'closed2'));
});
a.end();

[Trott]

What is the correct behavior when calling close(cb) on an already-closed fs stream?

• EBADF error
• do nothing
• fire the callback passed as an argument to close()

Asking for a friend.

[bnoordhuis]

@Trott I've been coming around to the idea that an EBADF exception is the most appropriate.

[evanlucas]

This is what I'm currently getting on v6.9.4:

var fs = require('fs');
var a = fs.createWriteStream('/tmp/aaa');

a.close(console.log.bind(null, 'closed'));
// prints "closed"
a.close(console.log.bind(null, 'closed'));

closed
closed
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: EBADF: bad file descriptor, close
    at Error (native)

Is that intended behavior now?

[mcollina]

@evanlucas I think that's not correct and we have a bug, a user should expect to receive that error in the callback, currently it is not.

[evanlucas]

wait, .close(cb) the cb can have an err argument? Has that always been the case?

[mcollina]

@evanlucas The correct way to end a stream is to call .end(). That deal with all the cases.
close() is not part of the Stream API, nor it's document anywhere for fs.
https://nodejs.org/api/fs.html#fs_class_fs_writestream.

So, if we think this is a needed feature, we should in fact document it, and add an error parameter to that callback. But I'm all in for deprecating close() for fs.createWriteStream(), and removing in two cycles from now.

However, we still need to document and fix ReadableStream.close because that's what is being used here: https://github.com/nodejs/node/blob/master/lib/fs.js#L2022.

If we look at: https://github.com/nodejs/node/blob/master/lib/fs.js#L1857, it's not doing anything.

Ahum.. I'll try to assemble a PR for this anyway.