-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix premature creation of file read streams #16
Fix premature creation of file read streams #16
Conversation
Read stream now created on the finish event for the write stream to the temp file. The finish event is emitted once the file is completely flushed. Also, the promise returned from the exported function isn't resolved until all files have been downloaded.
Codecov Report
@@ Coverage Diff @@
## master #16 +/- ##
==========================================
+ Coverage 94.11% 94.18% +0.06%
==========================================
Files 1 1
Lines 85 86 +1
Branches 6 6
==========================================
+ Hits 80 81 +1
Misses 5 5
Continue to review full report at Codecov.
|
Missed PR #14 which does the same thing... Anyhow, please look at this PR and merge and/or suggest changes. The master branch contains bugs. Try running the tests in this PR a couple of times without the suggested changes in index.js and you'll see what I mean. |
const tmpName = file.tmpName = new Date().getTime() + fieldname + filename; | ||
const saveTo = path.join(os.tmpdir(), path.basename(tmpName)); | ||
file.on('end', () => { | ||
const readStream = fs.createReadStream(saveTo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible that readStream is created before saveTo
is completely written.
Fixes issue #7 |
Same here #14 |
Good catch, thanks guys. |
@@ -56,8 +56,12 @@ module.exports = function (request, options) { | |||
|
|||
function onEnd(err) { | |||
if(err) reject(err); | |||
cleanup(); | |||
return resolve({fields, files}); | |||
Promise.all(filePromises) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty filePromises
should resolve
immediately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly and it does that as well. Any problems?
The following code prints "Promise result []"
Promise.resolve([]).then(console.log.bind(null,"Promise result"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance should be considered here, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a big sub-optimization. If you want performance you should rewrite to get rid of the temporary file stored in /tmp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Promise
with empty filePromises
is slower than resolve
immediately.
Smt like:
if (filePromises.length > 0) {
return Promise.all(filePromises)
.then(files => resolve({fields, files}))
.catch(reject)
} else {
return resolve({fields, files: []})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dominhhai, good point, but I am wondering if that is worth adding branching in the code. Let's put a pin on that, if perf ever becomes a problem, we can pull this back. Thanks for the insight!
Read stream now created on the finish event for the
write stream to the temp file. The finish event is
emitted once the file is completely flushed.
Also, the promise returned from the exported function
isn't resolved until all files have been downloaded.