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

Fix PaymentOptionsVC race condition where payment method loading race… #1476

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

yuki-stripe
Copy link
Collaborator

…s against setting defaultPaymentMethod.

We end up checking defaultPaymentMethod before initialization returns, leaving no room for the user to set the property.

This happens b/c

  • We make a network request for payment methods in init
  • That request can call the completion block synchronously if cached
  • We don't clear the cache in PaymentOptionsVC (unlike PaymentContext)

I find all of this surprising but to avoid breaking other things, I'm leaving all of these things as-is and making the smallest fix.

Motivation

#1475

Testing

Tested on UI Examples app, which always synchronously returns. Repro'd pre-fix, cant repro post-fix.

…s against setting defaultPaymentMethod.

This happens b/c
- We make a network request for payment methods in init
- That request can call the completion block synchronously if cached
- We don't clear the cache in PaymentOptionsVC (unlike PaymentContext)

I find all of this surprising but to avoid breaking other things, I'm leaving all of these things as-is and making the smallest fix.
@davidme-stripe
Copy link
Contributor

davidme-stripe commented Jan 15, 2020

A note from our offline chat about this: stpDispatchToMainThreadIfNecessary is kinda dangerous, as it isn't immediately obvious to the user whether it will execute immediately or on the next runloop. Changing stpDispatchToMainThreadIfNecessary to always call dispatch_async could cause side effects for existing users of the SDK, so we'll leave it alone for now, but we should probably reduce our usage of it.

@yuki-stripe yuki-stripe merged commit 166c66f into master Jan 15, 2020
@yuki-stripe yuki-stripe deleted the yuki/fix-default-pm-race-condition branch January 15, 2020 22:19
kgaidis-stripe added a commit that referenced this pull request Oct 31, 2022
Financial Connections: More polish around Native
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.

None yet

4 participants