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

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

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.

#2955

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();

fs: don't fail with EBADF on double close

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.

@Trott

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

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?

@evanlucas

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

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

@evanlucas

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

Fixed in b1fc774.

fs: avoid emitting error EBADF for double close

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

bnoordhuis commented on Sep 18, 2015

bnoordhuis commented on Sep 18, 2015

gkubisa commented on Sep 20, 2015

Trott commented on May 2, 2016

bnoordhuis commented on May 6, 2016

evanlucas commented on Jan 30, 2017

mcollina commented on Feb 7, 2017

evanlucas commented on Feb 7, 2017

mcollina commented on Feb 7, 2017

mcollina commented on Feb 13, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions