fs: don't fail with EBADF on double close

[bnoordhuis]

Calling fs.ReadStream#close() or fs.WriteStream#close() twice made it
try to close the file descriptor twice, with the second attempt using
the nulled out .fd property and failing with an EBADF error.

Fixes: #2950

CI: https://ci.nodejs.org/job/node-test-pull-request/342/

[thefourtheye]

I am not sure how the second close event is emitted. First time closed and fd is set as null. If self.fd !== number, then close is registered with open and returned immediately.

[bnoordhuis]

Both calls to .close() register the callback as a 'close' event listener that is emitted on the next tick. It may be leaning on implementation details a bit but but I think that's okay for a test.

[ChALkeR]

@bnoordhuis Is there a reason why this function receives an fd, checks self.fd, closes fd and sets self.fd to null?

It looks like the argument fd is never set. Could that one just be deleted, so the function will always use self.fd? Because the logic looks broken otherwise.

[bnoordhuis]

@ChALkeR It cannot. Look for the corresponding 'open' event ~90 lines up. The callback is sometimes called with an fd and sometimes not.

[ChALkeR]

@bnoordhuis Ok, true. But when it is — self.fd always equals to fd. That is not clear from just the close() function, but the function depends on that fact (without it, the behavior of the function would be very strange). Why not just use self.fd?

[bnoordhuis]

Virtual shrug. I didn't write that code and I'm not really inclined to change it for risk of breaking something. I could change the guard to if (fd === undefined && self.fd === null) return.

[ChALkeR]

@bnoordhuis Seems reasonable.

if (!fd && self.fd === null) would be at least corresponding to the next line. Though if (fd === undefined && self.fd === null) would also work for the current code.

Forget that, it's good as it is. LGTM.
If an extra check is included, it could break the purpose of this PR -- what if self.fd is closed before open(fd) gets processed? (Could that happen? I am not sure, can't check now.)

[bnoordhuis]

what if self.fd is closed before open(fd) gets processed?

If I understand you correctly, that can't happen, self.fd is assigned right before the 'open' event is emitted. It's nulled out the moment it gets closed so multiple listeners shouldn't be a problem, they either see a valid file descriptor or they see null.

[bnoordhuis]

Updated, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/382/

EDIT: Interestingly, the proposed change breaks the tests from this PR because the file descriptor is emitted as part of the 'open' event.

[brendanashworth]

Why not emit an error after it is already closed? Does calling .close() twice on a stream count as a programmer error? If yes, perhaps it shouldn't be thrown under the rug.

[jasnell]

@bnoordhuis ... still an issue?

[jasnell]

Does not appear to be happening in master or v5. Closing

[mscdex]

@jasnell I just tested this on v4 and v5 and the included tests do fail there.

[jasnell]

Ok. Then there's a fix that needs to be backported it seems. This particular PR is against master. If we can isolate the change that fixed it we can cherry pick that back.

[mscdex]

@jasnell I also just tried it on master and it fails there as well, so it looks like this is still applicable everywhere...

[jasnell]

Extremely odd. I am not seeing the same results. :-/ ... Ok, thanks for
double checking!

On Sun, Apr 10, 2016 at 5:07 PM, Brian White notifications@github.com
wrote:

@jasnell https://github.com/jasnell I also just tried it on master and
it fails there as well, so it looks like this is still applicable
everywhere...

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2955 (comment)

[jasnell]

Hmm.. odd... if I run the test by itself, it fails as reported. If I run the same sequence through the REPL, there's no error:

bash-3.2$ ./node
> 'use strict';
'use strict'
> 
> const fs = require('fs');
undefined
> 
> const s = fs.createReadStream('/Users/james/tmp/test.js');
undefined
> s.close(function() {});
undefined
> s.close(function() {});
undefined
> 

[mscdex]

@jasnell that makes sense because of the timing. The two .close() calls have to be made immediately after the stream is created (before an fd is assigned) so that the internal close() is added twice as open event handlers. When open gets emitted, the first close() starts the close request and sets the fd to null, then the second close() gets called right after that and causes the error.

[jasnell]

Ah, right. Ok. Makes sense.
On Apr 10, 2016 6:12 PM, "Brian White" notifications@github.com wrote:

@jasnell https://github.com/jasnell that makes sense because of the
timing. The two .close() calls have to be made immediately after the
stream is created (before an fd is assigned) so that close() is added
twice as open event handlers. When open gets emitted, the first close()
starts the close request and sets the fd to null, then the second close()
gets called right after that and blows up.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2955 (comment)

[Trott]

Been a while since this was run through CI, so...

CI: https://ci.nodejs.org/job/node-test-pull-request/2448/

[bnoordhuis]

In retrospect I don't think silently ignoring the second close call is a good idea. Closing.