bugfix for retry failing on subsequent launch

tus / TUSKit

The tus client for iOS.

https://tus.io/

MIT License

209 stars 114 forks source link

bugfix for retry failing on subsequent launch #169

Closed liyoung47 closed 1 year ago

liyoung47 commented 1 year ago

Hello, we are using TUSKit in our app and have encountered a problem with retry.

We are using version 3.2.0 (or rather latest main, 3.2.0 has a compile error which was fixed in main)

STR:

have a file fail to upload (using the wrong upload url is useful)
close application and relaunch
on launch, get .failedUploadIDs() and iterate over them calling .retry(id) Expected: TusClient attempts the upload again Actual: retry returns false because uploads[id] is nil

I see this reverts a recent change: https://github.com/tus/TUSKit/commit/2ad3209400c22d1c8760156d4a4168a44ad064e3 so perhaps this is not the correct solution, however I have tested with this change and it works both when retrying during the same session, and after closing and relaunching.

donnywals commented 1 year ago

Hi! Thanks for filing this. I'll take a look at the change and whether it re-introduces something that I had intended to fix with the reverted change 👍🏼

donnywals commented 1 year ago

I think to properly fix this we'll need to introduce a separate function to resume paused uploads. Historically retry has been called in our sample app for both cases so if we merge this we'd break retry. I will leave this PR open for now as a reminder to make a dedicated PR to address this problem and introduce a proper resume mechanism.

Acconut commented 1 year ago

Do I understand this issue correctly that findFailedUploadIDs does not return paused uploads, but just uploads where a network or HTTP error occurred?

donnywals commented 1 year ago

No, the issue is that we use retry for failed uploads as well is non-failed uploads while retry has some logic in place to check whether an existing upload exists. For a failed upload we won't have an in-progress upload and for a paused upload we do. Splitting retry into an explicit retry and resume (which will also more accurately reflect intent) is how we can fix this.

donnywals commented 1 year ago

@liyoung47 #172 should address this issue without breaking resume for paused uploads

liyoung47 commented 1 year ago

Thanks @donnywals ! Confirmed #172 fixes our issues ✨