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

Total Progress incorrectly reflected when keeping files in state. #4992

Open
2 tasks done
mok419 opened this issue Mar 13, 2024 · 4 comments
Open
2 tasks done

Total Progress incorrectly reflected when keeping files in state. #4992

mok419 opened this issue Mar 13, 2024 · 4 comments
Labels

Comments

@mok419
Copy link

mok419 commented Mar 13, 2024

Initial checklist

  • I understand this is a feature request and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

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()

const inProgress = files.filter((file) => {
  return (
    file.progress.uploadStarted ||
    file.progress.preprocess ||
    file.progress.postprocess
  )
})`

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()

const inProgress = files.filter((file) => {
  return (
    !file.progress.uploadComplete ||
    file.progress.preprocess ||
    file.progress.postprocess
  )
})

console.log('INPROGRESSNOTCOMPLETE', inProgress)

Alternatives

I think this solution is simple enough and won't break things!

@mok419 mok419 added the Feature label Mar 13, 2024
mok419 added a commit to mok419/uppy that referenced this issue Mar 13, 2024
Mentioned in my FR transloadit#4992

This allows not being uploaded files kept in state to not affect the total progress!
@mok419
Copy link
Author

mok419 commented Mar 13, 2024

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!

handleRestoreConfirmed = () => {
    this.uppy.log('[GoldenRetriever] Restore confirmed, proceeding...')
    // start all uploads again when file blobs are restored
    const { currentUploads } = this.uppy.getState()
    if (currentUploads) {
     //OLD CODE
      // this.uppy.resumeAll()
      // Object.keys(currentUploads).forEach((uploadId) => {
      //   this.uppy.restore(uploadId, currentUploads[uploadId])
      // })

      for (let uploadId in currentUploads) {
        const files = currentUploads[uploadId].fileIDs
        console.log('geloo', files)
        this.uppy.resumeAll()
        if (files.includes(this.currFileId)) {
          console.log('geloo', currentUploads[uploadId])
          this.uppy.restore(uploadId, currentUploads[uploadId])
          break
        }
      }

    }
    this.uppy.setState({ recoveredState: null }) 
  }

@mok419
Copy link
Author

mok419 commented Mar 13, 2024

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!

@Murderlon
Copy link
Member

Murderlon commented Mar 14, 2024

Thanks for reporting and diving into it!

I'll look at your PR.

I got things working by making Golden Retriever into its own file manager, which stores in memory which files are currently to be uploaded

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.

@mok419
Copy link
Author

mok419 commented Mar 14, 2024

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.
To make this work, I made my uppy max files 25 so we can store upto 25 files for recovery, but GR is aware of a different limit for current uploads.

My use case: (and a potential flaw I just realised)
User will be uploading 1 file in multiple tabs, I would like to detect if the user has previously tried to upload this same file and resume the upload if possible. If not, we can start a new upload, as when the user adds a new file to GR in-memory store, it internally calls uppy.add-file and I use the solution mentioned in #1122 to deactivate all other files in Uppy state, so when I call uppy.upload, only the files which have uploadStarted = false & uploadCompleted = true are uploaded. When the upload completes, I reset all the files as uploadStarted=false and uploadCompleted=true, so they are stored in local storage again for the next visit.

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!

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

No branches or pull requests

2 participants