Skip to content

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

Merged

Conversation

andrewjl-mux
Copy link
Contributor

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 work UploadManager will store MuxUpload and not ChunkedFileUploader.

UploadManager external-facing API is unchanged.

Root cause analysis

UploadManager exposes a means of subscribing to notifications when a managed MuxUpload starts and finishes network transport.

UploadManager stores ChunkedFileUploader 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 each ChunkedFileUploader.

Since ChunkedFileUploader is an internal class and the delegates of the UploadManager may be internal or external to the package, the delegate callback converts the list of ChunkedFileUploader to a list of MuxUpload by initializing a new MuxUpload for each key-value pair. A new MuxUpload gets initialized per delegate method call. Every MuxUpload emitted this way is different, when it comes to pointer identity, from the original MuxUpload initialized by the SDK client.

No MuxUpload coming from UploadManager 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 original MuxUpload, it replaces it with whatever it gets from UploadManager) then the original MuxUpload 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.

@andrewjl-mux andrewjl-mux requested a review from a team July 12, 2023 16:47
@@ -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;
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Contributor

@cjpillsbury cjpillsbury left a 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
@andrewjl-mux andrewjl-mux force-pushed the ajlb/remove-duplicate-muxupload-initialization branch from 8430449 to 322dee6 Compare July 12, 2023 18:46
@andrewjl-mux andrewjl-mux merged commit f52b59c into releases/v0.4.0 Jul 12, 2023
@andrewjl-mux andrewjl-mux deleted the ajlb/remove-duplicate-muxupload-initialization branch July 12, 2023 18:47
andrewjl-mux added a commit that referenced this pull request Jul 19, 2023
Fix stack overflow

uploadersByID -> uploadsByID

Add log
cjpillsbury pushed a commit that referenced this pull request Jul 20, 2023
Fix stack overflow

uploadersByID -> uploadsByID

Add log
andrewjl-mux added a commit that referenced this pull request Jul 20, 2023
Fix stack overflow

uploadersByID -> uploadsByID

Add log
@github-actions github-actions bot mentioned this pull request Jul 20, 2023
andrewjl-mux added a commit that referenced this pull request Jul 20, 2023
Fix stack overflow

uploadersByID -> uploadsByID

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

Successfully merging this pull request may close these issues.

2 participants