fd leak in ReadStream.destroy()

[rlidwka]

defensive programming is bad sometimes :)

this leaks:

setInterval(function() {
  var x = require('fs').createReadStream('/etc/passwd');
  x.destroy();
}, 1000);

this doesn't:

setInterval(function() {
  var x = require('fs').createReadStream('/etc/passwd');
  x.close();
}, 1000);

all details are here somewhere: pillarjs/send#55

I wonder how do I write tests for those...

[dougwilson]

The issue does not exist in Node.js 0.8; it looks like a Node.js 0.10 regression.

[dougwilson]

For all those who are concerned about this: use the destroy module on npm to destroy your streams instead of stream.destroy() :) It'll handle this (and future cases) correctly. Even if Node.js core fixes the issue, it'll of course still be a problem in all the previous v0.10.x releases.

[piscisaureus]

This looks like something that should just get fixed and back-ported to v0.10.

[dougwilson]

I meant, still an issue in 0.10.0 to 0.10.30 :)

[aheckmann]

@dougwilson do you mean the dethroy module?

[Fishrock123]

@aheckmann dethroy is now destroy: https://www.npmjs.org/package/destroy

[dougwilson]

Seems weird this fd leak fix didn't make it into 0.10.31 :/

[indutny]

@rlidwka perhaps we could have a test?

[rlidwka]

@indutny , how do you suggest to make one? I could do exec("lsof") (slow, non-portable, but a sure way), or try to mess with internal state (for example, try do close fd again on timeout and make sure it returns EBADF, but that's unreliable).

[indutny]

Hm... I wonder if we could limit number of fds programmatically. We do have ulimit -n-kindish call in src/node.cc, but nothing like that in tests... cc @tjfontaine

[dougwilson]

Any update on this? Will this ever make it into a 0.10 release? The leak makes it impossible to prevent fd leaks if you call destroy on ReadStreams. It basically means that ReadStream.prototype.destory is completely unusable in node.js 0.10 (the issue doesn't exist in 0.8).

[dougwilson]

How about updating the PR to use process._getActiveHandles() to verify?

[rlidwka]

How about updating the PR to use process._getActiveHandles() to verify?

Doesn't work, because leaked fds don't get listed in that array:
https://gist.github.com/rlidwka/7812f3fefd6f63c5bdb4

[dougwilson]

Darn

[dougwilson]

@indutny @tjfontaine any recommendations for this PR's tests? The PR fixes a regressions between node 0.8 and 0.10 where 0.10 will leak fds in a common usage with piping files in http servers.

[dougwilson]

Oh, oh, can we finally get this into a 0.10 patch release??? I see one is cooking up in the v0.10 branch. It'd be nice to have non-leaky things :)

[dougwilson]

Any test implementation suggestions yet?

[dougwilson]

@indutny @tjfontaine any suggestions for how the test can be done?

[dougwilson]

This is a big source of fd leaks in both 0.10 and 0.11.14. 0.8 code does not have this problem, because the regression was introduced after 0.8.

At the very least, this needs to be addressed before 0.12 if you're not going to fix it in 0.10.

[dougwilson]

cc @misterdjules

[jasnell]

@piscisaureus @indutny ... looks like this went stale but nothing was ever changed in v0.10 or v0.12.

[Fishrock123]

@jasnell this is fixed in io.js, I'll try to dig up the commit later.

[jasnell]

Yep saw that. We may want to backport the fix. But this particular PR can be closed.