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 mutex to stripe payment method creation #281

Merged
merged 9 commits into from
Jan 4, 2024

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Dec 29, 2023

Worked through this with @nfourtythree

Without a mutex, when creating a payment source, the webhook for payment_method.attach can come in before the payment source has been created, resulting in 2 payment sources, with the same token.

Questions for discussion

Unique commerce_paymentsources.token?

We need the mutex to cover this, but also begs the question, should it even be possible for commerce_paymentsources.token to be non-unique? I guess other gateways might not have unique identifier tokens?

Unnecessary saves

Even with the mutex, saving a payment source still results in the payment source getting saved a min of 3 times.

  • The original save/insert
  • payment_method.attach webhook
  • payment_method.updated webhook

It seems like we could be more judicious about bailing early in \craft\commerce\stripe\base\SubscriptionGateway::handlePaymentMethodUpdated to cut that down. Eg, if an existing payment source is found, and this is a payment_method.attach event, we can probably just bail and not save?

Furthermore, with the webhook will currently always overwrite the description, which you may not want.

Mutex timeout

Right now we have a timeout of 5 seconds when acquiring the mutex. It seems like that should be based on the Stripe webhook timeout, but I couldn't find that docuemented. From what I can guess, it is pretty quick, probably closer to 2 seconds.

Duplicate stripe events for "connect"

I don't know if this is a local dev problem only or not, but when using stripe listen as prescribed, you'll end up with duplicate webhooks for everything (one for "account", one for "connect").

@timkelty timkelty requested a review from a team as a code owner December 29, 2023 16:11
@nfourtythree nfourtythree merged commit 63f1829 into develop Jan 4, 2024
5 checks passed
@nfourtythree nfourtythree deleted the bugfix/payment-method-mutex branch January 4, 2024 16:33
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.

3 participants