-
Notifications
You must be signed in to change notification settings - Fork 10
Avoid duplicate MuxUpload initialization in UploadManager #61
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
Avoid duplicate MuxUpload initialization in UploadManager #61
Conversation
@@ -466,7 +466,7 @@ | |||
CODE_SIGN_STYLE = Automatic; | |||
CURRENT_PROJECT_VERSION = 1; | |||
DEVELOPMENT_ASSET_PATHS = "\"Test App/Preview Content\""; | |||
DEVELOPMENT_TEAM = CX6AHWLHM6; | |||
DEVELOPMENT_TEAM = XX95P4Y787; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(non-blocking): Were these changes to the DEVELOPMENT_TEAM
intentional? If no, revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this shouldn't be here, I'll revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The high level description of the PR makes sense to me (🙏 for the great overview). Code also looks reasonable (to the best of my ability, obv). Had a couple of nb callouts but otherwise 😎 good find/fix.
Fix stack overflow uploadersByID -> uploadsByID
8430449
to
322dee6
Compare
Fix stack overflow uploadersByID -> uploadsByID Add log
Fix stack overflow uploadersByID -> uploadsByID Add log
Fix stack overflow uploadersByID -> uploadsByID Add log
Fix stack overflow uploadersByID -> uploadsByID Add log
This PR contains a fix for
MuxUpload
progress reporting callbacks going silent mid-upload. The actual upload completes and is successfully delivered so the issue is limited to callbacks. To make this fix workUploadManager
will storeMuxUpload
and notChunkedFileUploader
.UploadManager
external-facing API is unchanged.Root cause analysis
UploadManager
exposes a means of subscribing to notifications when a managedMuxUpload
starts and finishes network transport.UploadManager
storesChunkedFileUploader
instances by key in a dictionary (hash map). When the dictionary content changes,UploadManager
emits a delegate callback and provides its delegates with an updated list of entities corresponding to eachChunkedFileUploader
.Since
ChunkedFileUploader
is an internal class and the delegates of theUploadManager
may be internal or external to the package, the delegate callback converts the list ofChunkedFileUploader
to a list ofMuxUpload
by initializing a newMuxUpload
for each key-value pair. A newMuxUpload
gets initialized per delegate method call. EveryMuxUpload
emitted this way is different, when it comes to pointer identity, from the originalMuxUpload
initialized by the SDK client.No
MuxUpload
coming fromUploadManager
will have its handler properties set with the SDK clients original closures. This triggers the bug.If the SDK client does not retain the
MuxUpload
it originally created (and the example code in our case does not retain the originalMuxUpload
, it replaces it with whatever it gets fromUploadManager
) then the originalMuxUpload
is deallocated because the SDK doesn't retain it either.This wasn't obvious when first introduced because the example code reset the callback handlers during each SwiftUI update cycle, the callbacks usually propagate an update unless a race condition is triggered: where a callback runs before a new handler is set but after the previous
MuxUpload
is deallocated.