fs / streams: file descriptor leak on connection abort

[bnoordhuis]

From nodejs/node-v0.x-archive#6041 (comment):

var http = require('http');
var fs = require('fs');

var filename = process.argv[0];
var filesize = fs.statSync(filename).size;

http.createServer(function(req, res){
  var file = fs.createReadStream(filename);
  res.writeHead(200, { 'Content-Length': '' + filesize });
  // uncommenting the line below fixes the fd leak
  //res.on('close', file.destroy.bind(file));
  file.pipe(res);
}).listen(8080);

Hit with ab but ^C before it completes and check the open files afterwards:

$ ab -c 1000 -n 1000 http://127.0.0.1:8080/
^C

$ lsof -p $(pgrep iojs) | wc -l
928

$ ab -c 1000 -n 1000 http://127.0.0.1:8080/
^C

$ lsof -p $(pgrep iojs) | wc -l
1928

$ ab -c 1000 -n 1000 http://127.0.0.1:8080/
^C

$ lsof -p $(pgrep iojs) | wc -l
2791

# etc.

I think we should assign some prio to this even if it's an old bug because it's a great way to DoS a server.

/cc @nodejs/streams

[vkurchatkin]

Is it a bug? I thought it was a known limitation of streams. It seems that the only way to solve this is to propagate errors upstream

[alexjeffburke]

Re the linked EMFILE issue I wonder if I hit something similar load testing a node server yesterday. I was doing exactly the same, pounding it with ab, and every once in a while EMFILE killed it. Was a little suspicious given a very short stack trace - IIRC TCP onconnection then events.js. If it is related it's been around aong time.. discovered I was still running 0.10.32. Anything I could do to help?

[chrisdickinson]

Yep, this is a known issue with piping fs streams into http responses. I'll investigate fixing this.

[brendanashworth]

Also see #1180. They seem to be the same bug but I'm not sure which to close (or leave both open). I'll mark this issue as a bug.

[evanlucas]

Is this something that could be fixed by something like:

diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index 99fa8ff..7aa362b 100644
--- a/lib/_http_outgoing.js
+++ b/lib/_http_outgoing.js
@@ -7,6 +7,7 @@ const util = require('util');
 const internalUtil = require('internal/util');
 const Buffer = require('buffer').Buffer;
 const common = require('_http_common');
+const fs = require('fs');

 const CRLF = common.CRLF;
 const chunkExpression = common.chunkExpression;
@@ -81,6 +82,12 @@ function OutgoingMessage() {
   this._headerNames = {};

   this._onPendingData = null;
+
+  this.on('pipe', (source) => {
+    if (source instanceof fs.ReadStream) {
+      this.on('close', source.destroy.bind(source));
+    }
+  });
 }
 util.inherits(OutgoingMessage, Stream);

or would it need to be fixed at the streams level?

[thomas-riccardi]

This would only fix this specific instance of the issue (http + fs).

The general issue is that close/destroy is not part of the Stream API. If it were, then Readable.pipe could have the additional role to close/destroy all streams of the pipe(s) on error.

This is what npm module pump tries to do, and with more than two piped streams too. But it's hard to do with no standard for close/destroy.

[dominictarr]

there is an defacto standard for close/destroy - all core streams have that, and many many userland streams get it for free via through or through2

[dominictarr]

although, adding error propagation into node streams would have unpredictable effects.

[jasnell]

@bnoordhuis ... I assume this is still an issue?

[bnoordhuis]

Yes, it's still unfixed.

[alexjeffburke]

Hmm given what has been mentioned about destroy(), does that mean this issue could do with a positive outcome in the discussion at #4401?

[ronkorving]

It's been a year, let's ask again...

@bnoordhuis ... I assume this is still an issue?

[mcollina]

I do not think that is a bug, it's how streams work. ab works hard on the TCP sockets, and causes a lot of them to abort, which will leave the file descriptors open. There is no current way to handle this case, and making .pipe() forward errors would cause too much unpredictable breakage.
This will never be fixed.

Now we have stream.destroy() in core, and we plan to port pump() as require('stream').pump, so that we can document the situation using only core.

We are tracking progress on this in: nodejs/readable-stream#283.

I'm 👍 to close this, or remove 'confirmed bug' label, as it is not a bug.

[mcollina]

Closing.

[dominictarr]

just for the record, pull-streams do not have this problem.
http://dominictarr.com/post/149248845122/pull-streams-pull-streams-are-a-very-simple
http://pull-stream.github.io/