Skip to content
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:temp file write timing issue #189

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

somewind
Copy link
Contributor

@somewind somewind commented Jan 8, 2020

#184 use promise to fix this bug

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage increased (+0.1%) to 98.425% when pulling 0a882ac on somewind:master into 2a467df on richardgirges:master.

@RomanBurunkov
Copy link
Collaborator

Hi @somewind ,

Thanks for this PR.
One thought I have about this: Have you considered to check whether the file has been flushed in mv function, instead of waiting resolving of all promises? In this case we don't need req.waits at all.

@somewind
Copy link
Contributor Author

somewind commented Jan 9, 2020

Hi @somewind ,

Thanks for this PR.
One thought I have about this: Have you considered to check whether the file has been flushed in mv function, instead of waiting resolving of all promises? In this case we don't need req.waits at all.

Delay waiting, which is a good idea, but I think writing file is fast. If you move the logic to mv, does it add complexity to the code?

@RomanBurunkov
Copy link
Collaborator

RomanBurunkov commented Jan 9, 2020

I mean wait for resolving finish promise in mv method individually instead of waiting resolving all promises after busboy finish event triggered. In such case we don't need waits array and all code stuff which mange waits array. Hope that clarify my idea.

@somewind
Copy link
Contributor Author

somewind commented Jan 9, 2020

I mean wait for resolving finish promise in mv method individually instead of waiting resolving all promises after busboy finish event triggered. In such case we don't need waits array and all code stuff which mange waits array. Hope that clarify my idea.

I fixed it, Please check changes again.

@somewind somewind closed this Jan 9, 2020
@somewind somewind reopened this Jan 9, 2020
@richardgirges
Copy link
Owner

This looks good to me @RomanBurunkov - I'm going to merge by EOD unless you have any reservations.

@somewind
Copy link
Contributor Author

I mean wait for resolving finish promise in mv method individually instead of waiting resolving all promises after busboy finish event triggered. In such case we don't need waits array and all code stuff which mange waits array. Hope that clarify my idea.

I have modified it as you said, please check the PR again, or if you have any other suggestions 😃

@RomanBurunkov
Copy link
Collaborator

I mean wait for resolving finish promise in mv method individually instead of waiting resolving all promises after busboy finish event triggered. In such case we don't need waits array and all code stuff which mange waits array. Hope that clarify my idea.

I have modified it as you said, please check the PR again, or if you have any other suggestions 😃

Yeap, I've seen that.
Probably my idea wasn't so good, since in case you need to do something with temp file before moving it u still have a chance to face with this issue.

@somewind
Copy link
Contributor Author

somewind commented Feb 17, 2020

I mean wait for resolving finish promise in mv method individually instead of waiting resolving all promises after busboy finish event triggered. In such case we don't need waits array and all code stuff which mange waits array. Hope that clarify my idea.

I have modified it as you said, please check the PR again, or if you have any other suggestions 😃

Yeap, I've seen that.
Probably my idea wasn't so good, since in case you need to do something with temp file before moving it u still have a chance to face with this issue.

How about making 'writePromise' available to the user?

You can use req.files.foo.tempFileWritePromise to wait, if you need to do something other than req.files.foo.mv with the temporary file

@RomanBurunkov
Copy link
Collaborator

@somewind

Cool idea, but looks too complicated to use.
Don't u mind to rollback this PR to the previous version with an array of uploads, looks that is the best option for now and I'm going to merge it.

@somewind
Copy link
Contributor Author

somewind commented Feb 21, 2020

@somewind

Cool idea, but looks too complicated to use.
Don't u mind to rollback this PR to the previous version with an array of uploads, looks that is the best option for now and I'm going to merge it.

OK, understand.

I have rolled back this PR to the previous version with an array of uploads, please check again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants