Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Large file downloads failing in 0.6.9 #2678

Closed
OrangeDog opened this Issue Feb 2, 2012 · 11 comments

Comments

Projects
None yet
4 participants

A rather complex server has a request handler that, after various async i/o, offers a large file for download.

var input = fs.createReadableStream(path);
response.pipe(input);

except with error handling etc.

However, on upgrading to Node 0.6.9 from 0.4.11, we noticed that the download is often mysteriously truncated with no errors logged except a Broken Pipe error in an stunnel instance. Investigation reveals that this occurs every time after two minutes when using curl (to Node directly, bypassing stunnel) if the option --limit-rate 50k is used. Successful download using Node 0.4.11 takes ~3:30 with the same curl command.

Some details in this gist. Note that in the failing case it is Node that sends the first FIN.
Can provide more (e.g. full pcap traces) if required.

Member

bnoordhuis commented Feb 2, 2012

Can you post a test case? I don't seem to be able to reproduce it.

Found the culprit, added to the gist. I guess you may not consider it a bug given what has been done, but I would not have expected the observed results.

koichik commented Feb 2, 2012

@OrangeDog - Because your write() does not return a value explicitly, it becomes undefined - it is a falsy. Therefore, pipe() suspends source stream. Probably, as a result, the socket is timed out. Please refer to API docs.

So, can you try like this?

    response.write = function(data, /*optional*/ encoding) {
        var result = originalWrite(data, encoding);
        request.totalLength += typeof data === 'string' ? Buffer.byteLength(data) : data.length;
        return result;
    };

isaacs commented Feb 3, 2012

@koichik Only exact boolean false will cause pipe() to pause. Returning undefined (or null or 0 or "") will not trigger a pause().

I think the issue is that you can have a condition where the file read stream emits "close", and this triggers res.destroy(), which abruptly closes the connection.

It's very tricky to trigger in a test, because you need to have a file read stream that's faster than the HTTP response, which requires a very slow client connection.

@OrangeDog Can you add this line and see if it prevents the error?

response.destroy = function () {}

It's not a good long-term fix, of course, but if it keeps the Bad Thing from happening, it'll be good information. The long-term fix is to comb through all the edge cases in our Stream api. This is a big todo item, definitely a 1.0 must-have, but not going to happen in 0.8 for sure.

Thanks.

koichik commented Feb 3, 2012

@isaacs

Only exact boolean false will cause pipe() to pause.

You are right, I should have gone to the bed before writing the comment (it was 4:00 a.m. in JST).

I think the issue is that you can have a condition where the file read stream emits "close", and this triggers res.destroy(), which abruptly closes the connection.

fs.ReadStream emits 'end' event before 'close', it tears down the pipeline. So, response.destroy() is not called I think. I still think that the return value of response.write() is the part of cause.

@isaacs, your suggestion had no effect
@koichik, yours fixed it

The fact that it works fine on 0.4.11 is presumably a bug in that implementation of pipe, rather than the new one?

@OrangeDog OrangeDog closed this Feb 3, 2012

koichik commented Feb 3, 2012

@OrangeDog - Thanks, I have just found the bug that is related to #2002. Reopening.

@koichik koichik reopened this Feb 3, 2012

koichik commented Feb 3, 2012

@isaacs - Can you review koichik/node@c4ebe64?

isaacs commented Feb 4, 2012

@koichik LGTM. Good find.

koichik commented Feb 5, 2012

@isaacs - Thanks, merging.

@koichik koichik closed this in 0f0af55 Feb 5, 2012

koichik commented Feb 5, 2012

@OrangeDog - Thanks for the report, fixed in 0f0af55.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment