-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Total Progress incorrectly reflected when keeping files in state. #4992
Comments
Mentioned in my FR transloadit#4992 This allows not being uploaded files kept in state to not affect the total progress!
I was going to submit a seperate feature request for how I got multiple files in recovery state working, but my method currently relies on the on.(file-added) and on.(upload-complete) listeners to set uploadStarted and uploadCompleted, so it is not something I can add into the package. I got things working by making Golden Retriever into its own file manager, which stores in memory which files are currently to be uploaded. I am working with S3 so not sure how my solution fits into other uploaders. But in simplicity when we emit restore-confirmed Golden Retriever can check if any files in its in-memory store have an upload id and can be resumed (rather than current implementation which attempts to upload all files in recovery). (This involved moving uppy.addFile and uppy.removeFile into GR, so when I add files, I add them to golden retriever, which stores in memory and then adds to uppy)! I only store 1 file in memory as that's what I need, but I'm sure it would work similarly for multiple files in memory too. Hope this helps for future if this is to be implemented!
|
Just found an edge case where if I don't reset the file isCompleted to false on adding a file to be restored, the progress is 0 until completed. (with the new TotalProgress calculation relying on !isCompleted) This also made me realise if the user has multiple tabs open, this will cause issues as they will alternate between setting isStarted and isCompleted between tabs and break the local storage state. Thankfully uppy keeps seperate tab state distinct which is good and means we don't need to worry about this, which is very good! It is a really complex problem to solve I am now realising that but I got it working pretty well, let me know if I can be of any help although my example is for a 1 file multipart s3 uploader. That is all I needed! |
Thanks for reporting and diving into it! I'll look at your PR.
This defeats the purpose of Golden Retriever, doesn't it? It can't be in-memory because it's meant to survive browser refreshes and restarts, which means everything is gone in memory. That's why it uses IndexedDB/Localstorage/ServiceWorkers. |
Currently from what I've seen is that when using Golden Retriever with Uppy, the local storage is loaded into Uppy state on page load. The single source of truth for a given tab is Uppy which keeps things in memory and writes back to local storage every 0.5 seconds (using GR). In the current Dashboard implementation, if the user chooses not to re-upload the same file, uppy.remove-file is called and the recovered file is completely deleted from Uppy and local storage. I wanted to preserve the recovery file upload state, even if the user chooses not to re-upload the same file right now. And hence I thought GR in-memory file management would be a good solution, as it keeps Uppy local storage state as the main source of truth, but uses a seperate in-memory store (in GR) to keep track of what the user is uploading in the current tab. My use case: (and a potential flaw I just realised) The flaw I just realised is testing the actual use case lol. I just started 3 concurrent uploads in different tabs and closed the window, only one of the tabs was written to local storage. This is race conditions, but I think I can get around this by changing how we write to local storage as I am reading it is atomic and thread safe across tabs! |
Initial checklist
Problem
As mentioned in #1112 (comment) When you want to keep files in state without uploading them by setting uploadStarted and uploadCompleted as true, the totalProgress is incorrectly reflected.
The totalProgress function (line 1408) currently looks at uploadStarted is true, and by doing the hack above, this makes all files currently in state be reflected in totalProgress updates.
`calculateTotalProgress(): void {
// calculate total progress, using the number of files currently uploading,
// multiplied by 100 and the summ of individual progress of each file
const files = this.getFiles()
I noticed this when I kept a large 500mb file in state and applied the solution above, I then went to upload a 10mb file alone and its progress gets stuck at very low total progress values (around 4/5%), then suddenly jumps to 100% as after my upload I set uploadStarted to 0 and it is regarded as false.
Solution
change the total progress from looking at uploadStarted to !uploadCompleted
This solved the issue for me! Not sure how it will affect the package!
calculateTotalProgress(): void {
// calculate total progress, using the number of files currently uploading,
// multiplied by 100 and the summ of individual progress of each file
const files = this.getFiles()
Alternatives
I think this solution is simple enough and won't break things!
The text was updated successfully, but these errors were encountered: