pionl / laravel-chunk-upload

The basic implementation for chunk upload with multiple providers support like jQuery-file-upload, pupload, DropZone and resumable.js
MIT License
600 stars 168 forks source link

Race condition in FileMerger / ParallelSave #168

Open Jimbolino opened 5 months ago

Jimbolino commented 5 months ago

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 commented 5 months ago

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?