fs: fix fd leak in ReadStream.destroy()

[jonathanong]

this is a file descriptor leak PR that was opened in joyent/node for a while but had no traction. please pull this or give us directions on the tests!

nodejs/node-v0.x-archive#8178

/cc @rlidwka @dougwilson

[dougwilson]

yay!

[rvagg]

existing tests all pass with this change? and do you know the reasoning behind the addition here (i.e. have you done a blame here or on readable-stream to figure that out?)

[dougwilson]

@rvagg all existing tests passed, yes. The change was part of this commit: nodejs/node-v0.x-archive@44b308b There is no indication for the purpose except that it's a regression that causes code that calls destory() on a ReadStream to leak the fd if the fd didn't happen to open by the time destroy() was called. 0.8 didn't have this behavior since it just called close(), which already does the appropriate checks and even waits for the fd to become available so it doesn't leak.

The original issue was acknowledged as a bug, but it was told there needed to be a test, but how to do that cross platform was never answered, since Node.js does not give access to the list of all open file descriptors.

[rvagg]

Here's your test case, pull it in to this PR: rvagg@3d89deb

[rvagg]

removed a setImmediate() that was slightly obscuring the problem, use rvagg@69b7102 instead

[jonathanong]

@rvagg i love you

[rvagg]

lgtm, obviously, need a review from someone else and then I'll deal with the multi-commits appropriately

[indutny]

Also, the test and patch should come in one commit.

[indutny]

I'm not sure what exactly we are fixing with this, btw. The destroy method appears to be undocumented, and from what I do see from source of lib/fs.js - it is used in a right way at the moment.

.destroy() method is sort of a raw thing and people should know the right way to call it. .close() is more like a public thing for a stream as it handles various internal states and makes them invisible to users.

Additional argument: .destroy() on net.Socket behaves in a same way as .destroy() on ReadableStream at the moment.

I'm generally -1 for this patch, unless a good argument will appear here. +1 for fixing the documentation of fs module and making a note about destroy behaviour and open event.

[dougwilson]

@indutny here is the bug:

1. I create a stream or get it from somewhere.
2. I pipe that stream into a WritableStream that I created fs.createWriteStream. WriteStream.prototype.destroy is set to ReadStream.prototype.destroy (https://github.com/joyent/node/blob/v0.10.24/lib/fs.js#L1689)
3. Now WriteStream is busy trying to get a fd from the OS
4. The stream I piped into the WriteStream closed because of an error before the fd was acquired.
5. Node.js calls WriteStream.prototype.destory at https://github.com/joyent/node/blob/v0.10.24/lib/stream.js#L87 ; I'm not the one calling destroy. Now the WriteStream leaked a fd.

TL;DR the pipe implementation from Node.js is what is calling .destory on the stream and not .close and it can do it at the wrong time since it doesn't know any better.

[indutny]

@dougwilson v0.12 streams are not calling destroy, AFAIK. And your fix is targeting v0.12 branch.

[jonathanong]

@rlidwka @rvagg did one of you guys want to take over this PR (close this, open your own branch)? otherwise i'll look into this this weekend

[dougwilson]

v0.12 streams are not calling destroy, AFAIK. And your fix is targeting v0.12 branch.

Clearly, it is: https://github.com/iojs/io.js/blob/v0.12/lib/stream.js#L89

[indutny]

@dougwilson am I right that this is the legacy streams API, and is not used by fs.WriteStream?

[indutny]

Actually v0.10 streams are not calling destroy either, but I didn't check it that much.

[dougwilson]

I'm not sure what you're asking, I'm sorry. This is what I'm doing:

var ws = fs.createWriteStream(...)
stream.pipe(ws)

Now stream emits close before the ws has acquired the fd and now ws will never close the file descriptor, since pipe from Node.js core called ws.destroy (yes, in 0.10 and 0.12). You need to look in the pipe implementation, which is in the lib/stream.js file (exact lines linked in various comments above).

[indutny]

@dougwilson why that stream is using legacy pipe function? The new one lives there: https://github.com/iojs/io.js/blob/v0.12/lib/_stream_readable.js#L468 and is not using .destroy(). AFAIK, destroy is not a part of Streams API, and should not really be called by anyone.

[rlidwka]

@rvagg 's test could be simplified to this:

var fs = require('fs');
var assert = require('assert');
var fdClosed = false;

fs.close = (function(close) {
  return function() {
    fdClosed = true;
    close.apply(fs, arguments);
  }
})(fs.close);

fs.createReadStream(__filename).destroy();

process.on('exit', function() {
  assert(fdClosed);
});

It will fail without and succeed with this patch. But it's implementation-dependent, that's what worried me. I mean, it tests that fs.close is called, but doesn't actually test the leak itself (even though right now it happens to be the same thing).

[dougwilson]

@indutny it's what I'm being given by old 0.8-style code from a vendor, as they are inheriting from the old Stream class.

Stream.prototype.pipe exists in the Node.js core codebase. So it sounds like Node.js core is not compatible with itself and even if I get a stream that gets it's pipe method from within the Node.js core code, you're saying I'm not allowed to turn around and use it with fs.createWriteStream? That sounds very odd to me. Is there a way I can fs.createWriteStream and get something that is compatible with the old stream implementation that is still sitting in the Node.js core code, then?

[indutny]

@rlidwka see the thing I am talking about is that .destroy() won't be called in case of correct Stream API usage. In fact, this method is undocumented, and there is no reason for exposing it, since we have .close().

[rlidwka]

AFAIK, destroy is not a part of Streams API, and should not really be called by anyone.

In this case it should probably be renamed to _destroy.

But what the accepted method to close a stream is? As far as I know, close only exists for fs streams.

[indutny]

@rlidwka yes, it is .close().

[indutny]

@dougwilson I don't see any reason to keep Stream.prototype.pipe, let's remove it. cc @bnoordhuis @trevnorris @rvagg

[indutny]

@rlidwka not sure about removing it either. What are your thoughts on this @bnoordhuis ?

[dougwilson]

I don't see any reason to keep Stream.prototype.pipe, let's remove it

This is 100% fine with me for 0.12. Can we do anything for 0.10, perhaps, pretty please?

[indutny]

@dougwilson yeah, I guess I'll land your patch :) Could you transform the test to make it explicitly use old streams API. It wasn't really evident what kind of problem you had from the test case that you are submitting ;)