Ensure File#createReadStream sockets are properly closed on error

[ryanseys]

Trying to address #523

Don't merge this, it's not complete.

This seems to mitigate sockets staying open when a request is ended early (only scenario I tried to get the request to hang).

Question for @robertdimarco: What do you mean by:

and hang-up part-way through that file

in your comment here. I don't know how to "hang up" part way, and is it the server that is hanging up or the client? (I'm assuming client). Any other scenarios you can think of that we should test?

@stephenplusplus Seems that done at the bottom of File#createReadStream isn't called when throughStream gets an error event like you can see in this PR. I listen for this event specifically and destroy the socket related to the request at that time. This seems to destroy all the sockets laying around and fires an error events saying there is a "Download mismatch" according to verification.

My test code can be found here: https://gist.github.com/ryanseys/0e2c3244175db6525d7a

Set maxSockets: 2 when running test code because currently infinite sockets are allowed to remain open so the 4th request will succeed.

I have a feeling this is on the right track but I perhaps don't clean up everything appropriately due to the craziness that is Node.js Streams and request. In any case, it still seems to mitigate the issue a bit so far.

[robertdimarco]

@ryanseys I'm referring to a client disconnect before the write-stream was able to complete. We came across this issue by pointing a browser at the proxy server, let the browser connect and begin downloading, and then close the tab / refresh / etc. to cause the client to disconnect the socket. That is generally sufficient to reproduce the issue.

The handling for this I imagine is a little tricky, because the error might not be propagated from Express's write-stream to the read-stream / through-stream returned from file.createReadStream(). That part around piping likely just needs some documentation around it, as other devs will have to handle that client disconnect and propagate to this module.

[ryanseys]

The problem with using that scenario (i.e. closing a tab) is I have no idea what that means in regards to what happens to either the read or write stream. A code-only test to reproduce this is most ideal, especially for ensuring this issue doesn't arise again as a regression. That's what I'm trying to accomplish here with my test script. I will try your Express method and watch the events that occur on the streams to get an idea of where things can go wrong so we thoroughly patch this thing.

[robertdimarco]

@ryanseys Understood. Let me know if I can be of any help.

[stephenplusplus]

@ryanseys I have a bit of a different approach going on: stephenplusplus@7b4ab4f - can you try this out with your tests?

[ryanseys]

@stephenplusplus That's much cleaner and appears to work with my tests when maxSockets: Infinity (no open sockets at the end). When maxSockets: 2, weird things happen though. The third request that get's prematurely ended doesn't throw an error, or the 4th request that should succeed just never does, BUT there are no sockets left open as well. Considering we are going to have maxSockets: Infinity this shouldn't be an issue.

[ryanseys]

Actually that might be just because maxSockets is doing its job, in which case we just set it to Infinity and hope nobody has more than Infinity simultaneous sockets open on gcloud (but if this happens, that person should probably get a nobel prize of the node.js variety).

[ryanseys]

tl;dr: looks good to me. I'll cherry pick into this PR.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[ryanseys]

@googlebot confirm.

[ryanseys]

Apparently @googlebot doesn't like you @stephenplusplus

[stephenplusplus]

Neither does Travis. I'm not big with robots, I guess :/

[stephenplusplus]

@ryanseys I suppose I should fix the tests, since I'm the one who wrecked your PR!

[stephenplusplus]

Carrying this on in #636.