andrewrk / node-s3-client

high level amazon s3 client for node.js
MIT License
1k stars 305 forks source link

Unexpected end of input #97

Open baramuse opened 9 years ago

baramuse commented 9 years ago

Hi,

I'm not still quite sure the issue I'm facing in my code is coming from this library and as it seems to be a race condition on I/O it's hard to reproduce, although my investigation lead me to the downloadFile function. Basically I'm downloading a folder with this library, then (straight away) read a JSON file from the downloaded folder and I do sometime get a Unexpected end of input. Although if I go and check the file, everything looks fine so I'm thinking at the time the JSON parser tried to parse the file maybe it wasn't all available.

In the downloadFile function, I can see that the iteration takes place on the end or close event of the writableStream https://github.com/andrewrk/node-s3-client/blob/master/lib/index.js#L476-L481

request.on('httpDone', function() { 
          if (errorOccurred) return;
          downloader.progressAmount += 1;
          downloader.emit('progress');
          outStream.end(); 
          cb();
        })

https://github.com/andrewrk/node-s3-client/blob/master/lib/index.js#L516-L519

outStream.on('close', function() {
          if (errorOccurred) return;
          hashCheckPend.wait(cb);
        });

Wouldn't it be more accurate to hook on the finish event, so we are sure the that all data has been flushed to the underlying system

something like (i'm not exactly sure here :) )

request.on('httpDone', function() { 
          if (errorOccurred) return;
          downloader.progressAmount += 1;
          downloader.emit('progress');
          outStream.end(function () { cb(); })); 
        })
faceleg commented 9 years ago

Though I'm a contributor I have little familiarity with this portion of the code.

I can, however, review and merge pull requests, provided they prove that the tests all pass.

Hint hint :+1:

faceleg commented 9 years ago

But yes, your logic is sound and doing it this way makes more sense (and likely should resolve the errors you are experiencing).

faceleg commented 9 years ago

I would make the change, commit it to a feature branch, run the tests.

If the tests pass, I would then continue working on my project keeping an eye out for the same issue.

If after a few days the issue did not recur, I would submit a PR and call it it fixed.