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

Race condition in FileMerger / ParallelSave #168

Open
Jimbolino opened this issue Mar 25, 2024 · 1 comment
Open

Race condition in FileMerger / ParallelSave #168

Jimbolino opened this issue Mar 25, 2024 · 1 comment

Comments

@Jimbolino
Copy link

Currently we are using a custom dropzone js implementation to upload large files.
When we are posting 3 chunks concurrently, we are getting weird errors. (files incomplete, unable to move, unable to delete, etc)

The problem disappears when we limit the uploader to 1 thread or when we add a sleep($chunkId); on our server. So this would point to a race condition.

I'm not sure exactly where/how the problem could be fixed best, but there are a few points that can cause problems:

https://github.com/pionl/laravel-chunk-upload/blob/master/src/Save/ParallelSave.php#L98
the "getSavedChunksFiles" function does not know if all chunks have completely finished copying.

https://github.com/pionl/laravel-chunk-upload/blob/master/src/FileMerger.php#L47
FileMerger has the same problem, it cannot know if all the chunks that it is merging, are full and complete

I think the root cause of the problem might be that move_uploaded_file does not give any guarantees.
I've read somewhere that the function can either rename or copy+delete.
In our setup we are using a custom ChunkStorage on a NFS mount (because we have a multi-server load balancer setup), so i assume the file move_uploaded_file call is using the copy+delete method.

A possible solution might be to move() the file to the correct destination, but append a .tmp to the filename.
And then rename() the file directly after that.
This would prevent the FileMerger from merging files that are not finished copying.

@pionl
Copy link
Owner

pionl commented Mar 27, 2024

Hi @Jimbolino , thank you for the detailed points. I do not have the proposed setup but it makes sense.

Do you think you can make the changes to improve the upload + merge to fix the issue for you?

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

No branches or pull requests

2 participants