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

Add dispatch_async to [STPRedirectContext handleWillForegroundNotification] #962

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

joeydong-stripe
Copy link
Contributor

@joeydong-stripe joeydong-stripe commented Jun 8, 2018

Add dispatch_async to [STPRedirectContext handleWillForegroundNotification] to allow [STPRedirectContext handleURLCallback] to be prioritized

We considered some other approaches and landed on this one:

Option 1:

Delete STPURLCallback completely

The downside to this approach is the host app will still get [AppDelegate application:openURL:options:] calls. If STPURLCallback was gone, the host app won't know whether or not that [AppDelegate application:openURL:options:] call should be ignored by their own app's routing logic.

Option 2:
Don't unschedule STPURLCallback and let the STPRedirectContext stay in the STPRedirectContextStateCompleted state

The downside to this approach is the STPRedirectContext ends up holding a non-negligible amount of memory in order to just store the STPRedirectContextStateCompleted state.

Option 3:

Similarly to the last one, we can avoid unscheduling the STPURLCallback for a second or so.

This is very similar to the approach we landed on but would require more significant code changes. Also the reasoning behind how much delay to have is unclear.

@joeydong-stripe
Copy link
Contributor Author

r? @csabol-stripe @danj-stripe

@joeydong-stripe
Copy link
Contributor Author

Intended solution to the second problem described in #957

…fication]` to allow `[STPRedirectContext handleURLCallback]` to be prioritized

We considered some other approaches and landed on this one:

Option 1:

Delete `STPURLCallback` completely

The downside to this approach is the host app will still get `[AppDelegate application:openURL:options:]` calls. If `STPURLCallback` was gone, the host app won't know whether or not that `[AppDelegate application:openURL:options:]` call should be ignored by their own app's routing logic.

Option 2:
Don't unschedule `STPURLCallback` and let the `STPRedirectContext` stay in the `STPRedirectContextStateCompleted` state

The downside to this approach is the `STPRedirectContext` ends up holding a non-negligible amount of memory in order to just store the `STPRedirectContextStateCompleted` state.

Option 3:

Similarly to the last one, we can avoid unscheduling the `STPURLCallback` for a second or so.

This is very similar to the approach we landed on but would require more significant code changes. Also the reasoning behind how much delay to have is unclear.
Copy link
Contributor

@danj-stripe danj-stripe left a comment

Choose a reason for hiding this comment

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

Looks good. I never feel good about using dispatch_async to re-order execution to work around undocumented behavior when we have implicit coupling, but I think this is our best option.

@joeydong-stripe
Copy link
Contributor Author

Looks good. I never feel good about using dispatch_async, but I think this is our best option.

Yeah same here. Surprising to see a case where it's appropriate.

@joeydong-stripe joeydong-stripe merged commit cbed7bb into master Jun 11, 2018
jaimepark-stripe pushed a commit that referenced this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants