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

Clear iOS platform instances synchronously #249

Merged
merged 9 commits into from Nov 29, 2021

Conversation

ben-xD
Copy link
Contributor

@ben-xD ben-xD commented Nov 22, 2021

I'm moving this code from running in the background, to instead run synchronously.

This fixes a race condition bug where dispose is first called from the Dart side to clear all the instances of realtime and rest. At this point, the realtime/rest clients are not yet cleared, and the work is queued. The actual implementation of dispose might only happen afterwards, because of dispatch_async. By that time, a user might have created a new realtime instance. Then, this dispose method finally gets called, and their client is deleted, after they create it.

This was actually discovered because I removed the button create realtime, and created the realtime client on launch instead. User applications would likely create a realtime instance either on application launch or user log in too.

Having said that, adding buttons for closing realtime and re-creating it can be useful to debug issues.

@ben-xD ben-xD added the bug Something isn't working. It's clear that this does need to be fixed. label Nov 22, 2021
@ben-xD ben-xD self-assigned this Nov 22, 2021
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 22, 2021 11:06 Inactive
@ben-xD ben-xD changed the title Run dispose implementation (clearing rest/realtime client instances) immediately, not with dispatch_async Run dispose implementation (clearing rest/realtime client instances) immediately, not later (dispatch_async) Nov 22, 2021
@ben-xD
Copy link
Contributor Author

ben-xD commented Nov 22, 2021

Here is an explanation of the existing buggy code:

When a users application (or example app) launches for the first time, or a hot restart happens (Flutter app restarts without restarting iOS host application), the Dart side calls a method: registerWithCompletionHandler. This calls AblyFlutter disposeWithCompletionHandler.

Bug: The registerWithCompletionHandler method is first called when the app launches, but the clearing does not happen immediately. Then, the user might programmatically create a realtime at app launch too. This saves a realtime client in AblyFlutter.m's _realtimeInstances. However, finally, the following code is executed in the background:

    for (ARTRealtime *const r in _realtimeInstances.allValues) {
        [r close];
    }
    [_realtimeInstances removeAllObjects];
    [_restInstances removeAllObjects];

deleting the users realtime client, when they didn't intend for it to be deleted.

@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 22, 2021 11:21 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 22, 2021 11:24 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 22, 2021 11:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 22, 2021 12:17 Inactive
@ben-xD ben-xD changed the base branch from enhancement/platform-encapsulation to enhancement/log-level-enum November 23, 2021 11:28
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 23, 2021 11:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 23, 2021 15:21 Inactive
@ben-xD ben-xD changed the base branch from enhancement/log-level-enum to enhancement/rename-client-store-methods November 23, 2021 15:30
@github-actions github-actions bot temporarily deployed to staging/pull/249/dartdoc November 23, 2021 15:33 Inactive
@ben-xD ben-xD changed the title Run dispose implementation (clearing rest/realtime client instances) immediately, not later (dispatch_async) Clear instances synchronously Nov 29, 2021
@ben-xD ben-xD changed the title Clear instances synchronously Clear iOS platform instances synchronously Nov 29, 2021
Base automatically changed from enhancement/rename-client-store-methods to main November 29, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants