Streams not properly being closed for static files when browser gets cache hit

[chiahrens]

If the static file is retrieved properly, everything goes through the normal flow where the Stream._transmit is invoked and the stream is closed after the data is piped.

However, if there is a cache hit from the browser on the static file, Hapi executes Generic._transmit, and the readStream opened in response/File is never closed. This eventually results in an error that terminates node:
Error: EMFILE, open '/private/tmp/UIaaS/presentation/serving/lib/client/componentPageMixins.js'

I was able to reproduce this with the following combo:
Node v0.10 with Hapi 0.16
Node v0.10 with Hapi 1.0.0 (cloned from master as of yesterday morning)

[hueniverse]

What is "a cache hit from the browser on the static file"?

[chiahrens]

The browser requests a static file, but once it gets the timestamp from the metadata, the browser abruptly closes the connection. It looks like the stream is created, but since it's not processed, Node is holding on to the filehandle. I'm looking for a workaround now, but all the node events from 0.8 don't seem to be available or have changed.

[hueniverse]

Looking.

[chiahrens]

My bad. I didn't realize request.reply() linked to the detailed doc on result. Thanks.

[chiahrens]

Sorry. I meant to close the other ticket on docs.

For this, I was able to come up with a quick workaround, but it involved modifying hapi code. If I add the following to response/file.js:

        var stream = Fs.createReadStream(self._filePath, {autoClose: true});
        + request.closeTheseStreams = request.closeTheseStreams || [];
        + request.closeTheseStreams.push(stream);

And then add a tail handler in my code:

    server.on('tail', function (request) {
        if(request.closeTheseStreams && request.closeTheseStreams.length > 0){
            request.closeTheseStreams.forEach(function(s){
                s.close();
            });
        }
    });

It seems to be closing the file handles.

[hueniverse]

{autoClose: true} is already the default. Looking at the rest now.

[hueniverse]

What browser are you using? Trying to reproduce this in a simple app.

[chiahrens]

Chrome. I have my default ulimit set to like a million. So I open a console, do ulimit -n 25, start node, and then hit a static file in Chrome and keep doing refresh until it starts erring - usually like 15 refreshes.

[hueniverse]

Try this and let me know if it works.

[chiahrens]

That doesn't work because at some point, when the browser abruptly disconnects, hapi decides to use the generic response after the file stream is created instead of the stream response, so none of that code is executed.

Here is a simple test that I used:

1. mkdir -p test/htdocs && echo "{}" >> test/htdocs/test.css && cd test
2. Create a file app.js with the following:

var Hapi = require('hapi');
var server = new Hapi.Server('0.0.0.0', 8080);
server.route({
    method: 'GET',
    path: '/{path*}',
    handler: {
        directory: { path: "htdocs"}
    }
});
server.start();

3. npm install hapi (from master)
4. node app.js
5. In another console, run "ps -ef|grep node" to get the pid of the node process. Use that pid and run 'lsof -p |grep "test.css"' to get the test.css file descriptors being held open by that process.
6. In the console, do "curl http://localhost:8080/test.css" and when you check the number of open files with lsof, you will see that they're being closed properly.
7. Now go to chrome, and browse to curl http://localhost:8080/test.css. Hit refresh a few times and run the lsof command again, and you will see that the open files just being held by the process.

[chiahrens]

So it's this code in index.js that causes it to bypass the stream._transmit, and instead calls generic._transmit if file has not been modified:

        if (ifModifiedSinceHeader &&
            lastModifiedHeader) {

            var ifModifiedSince = Date.parse(ifModifiedSinceHeader);
            var lastModified = Date.parse(lastModifiedHeader);

            if (ifModifiedSince &&
                lastModified &&
                ifModifiedSince >= lastModified) {

                var unchanged = new internals.Empty();
                unchanged._code = 304;
                return prepare(unchanged);
            }
        }

[chiahrens]

Oh yeah, so I was completely wrong about the abruptly disconnecting part. It's actually a graceful disconnect after hapi checks the timestamp. I was assuming that this was Node's behavior based on my past experience with other similar app servers that did abruptly abort connections.

[chiahrens]

The following change in response/index.js is a working hack:

                unchanged._code = 304;
                + response._stream && response._stream.close && response._stream.close();
                return prepare(unchanged);

Strangely, stream.destroy() doesn't seem to close the filestream.

[hueniverse]

@chiahrens Thanks for your help with this. I've verified the fix with your scenario. The file stream is no longer opened unless it is being used.