Simultaneous webhook requests #1735
Replies: 7 comments 20 replies
-
|
@dbartenstein The way the sync part works is that we first check if the object exists or not, if it doesn't then we proceed to create it. When 2 webhooks that have the same related objects hit the code very closely, it can happen that the related object doesn't exist when they check for it but by the time one of them gets to the creation part, another webhook has already created the object and hence you get the error. I've been meaning to address this issue for some time now but some other issue or feature has always taken priority. I'll get to this soon enough. In summary, it's a webhook race condition. One very naive solution is to force the webhooks to get executed sequentially but that would affect the performance a lot. But this would be extremely reliable. The other solution is to enforce checking if the object exists before creating it always. To me this seems like the way forward but not sure if I'm missing any corner cases or not. This would be more of a band aid solution to just this particular race condition. |
Beta Was this translation helpful? Give feedback.
-
@arnav13081994: Where exactly would you hook in? We’re looking into executing webhook events of the same webhook in sequence using Redis locks. Line 40 in 8b8f601 |
Beta Was this translation helpful? Give feedback.
-
|
We now enforce serializing events per webhook using https://github.com/ionelmc/python-redis-lock and today made a release with it.
So far this looks very good and we are able to process a lot more webhook requests! |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 thanks for your reply! We now have enabled And yes, we are looking forward to a fix of this webhook race condition. |
Beta Was this translation helpful? Give feedback.
-
|
@dbartenstein I don't think you'll be able to get around this race condition that way, but let's see. Please keep me posted. The fix needs to be in this library for that. |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 we have further reduced the Stripe webhook requests to 1 request per second. Additionally we have greatly reduced the webhook events which are fired, especially Stripe invoiceitem. Now the error messages are mostly gone. Still a proper fix for the webhook race condition would be highly appreciated. Note: we even experienced dramatic database locks which led to an unstable server when no request limits and webhook event limiting were in place. |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 we still keep receiving messages such as: How do you think this can be best solved? |
Beta Was this translation helpful? Give feedback.
-
@arnav13081994: here’s the traceback: |
Beta Was this translation helpful? Give feedback.
-
|
@dbartenstein Quick Question: Do these events eventually succeed? Stripe does keep retrying even test events for up to 3 hours. Also in the above traceback, it seems the error is caused due to running of File "/home/zemtu/.virtualenvs/django22_py39/lib/python3.9/site-packages/django/dispatch/dispatcher.py", line 181, in <listcomp>
(receiver, receiver(signal=self, sender=sender, **named))
File "/home/zemtu/www/zemtu/billing/stripe/signals.py", line 94, in update_invoice
associated_invoice.save()It looks like you are trying to do 2 Please correct me if I'm wrong about this. If my interpretation is correct, you should probably try to check if |
Beta Was this translation helpful? Give feedback.
-
@arnav13081994 @jleclanche thanks! I will look into it! Edit: let me share the complete |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 for your information: we now create a delayed Celery task for updating the associated invoice to prevent that DjStripeInvoice and our invoice are updated in the same transaction, hopefully preventing deadlocks. |
Beta Was this translation helpful? Give feedback.
-
|
@dbartenstein I think that's a step in the right direction. Another approach would be to add an artificial sleep of 0.5 seconds to delay the processing. But that's a hack (and that too not a good one) |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 let me share the current status with you: In the next couple of days we’ll see if this new configuration solves the issue. Let us know what you think about our NGINX configuration and if you see further room for improvements. |
Beta Was this translation helpful? Give feedback.
-
|
@dbartenstein Does your configuration solve the issue? |
Beta Was this translation helpful? Give feedback.
-
|
@roniemartinez it solves the issue at the cost of some failed Stripe webhook events, which are retried later. Additionally we adapted our code so that triggered calls are as short as possible to prevent them from running too long. If additional operations have to be done, we put them on our Celery queue to be executed asynchronously. |
Beta Was this translation helpful? Give feedback.
-
|
@roniemartinez we have taken the next step: #1735 (reply in thread) |
Beta Was this translation helpful? Give feedback.
-
|
@arnav13081994 @jleclanche |
Beta Was this translation helpful? Give feedback.
-
|
I don't think there is a way right now. It should be easy enough to build by storing timestamp comparisons. |
Beta Was this translation helpful? Give feedback.
-
|
@jleclanche I guess it's time we started working on update and create interfaces and handle these things internally in Djstripe. What do you think? |
Beta Was this translation helpful? Give feedback.
-
|
@jleclanche @arnav13081994 yes, I also think that it’s best to handle these things internally in the djstripe core. It seems like people already start doing their own workarounds: #1267 (comment) |
Beta Was this translation helpful? Give feedback.
-
|
I agree. @arnav13081994 if you're going to work on this please post a proposal first though. |
Beta Was this translation helpful? Give feedback.
-
|
I ran into the deadlock issue, too. To work around it during development I followed @dbartenstein's suggestion to use nginx to limit requests. Here is my nginx.config and docker-compose.yml docker-compose.yml rate_limit_stripe_wh_nginx.conf: Then use the stripe forwarding as follows: This way I can continue developing without any problems. |
Beta Was this translation helpful? Give feedback.
-
|
I noticed some requests fail with error 502/503 using above method. So, please use with care. |
Beta Was this translation helpful? Give feedback.
-
|
@btzs yes, we also have the issue that Stripe webhook requests are not processed immediately, but after some retries. And yes, that could lead to some events not being processed when all retries fail as well. @jleclanche @arnav13081994 what would be the best approach for solving this? |
Beta Was this translation helpful? Give feedback.
-
|
Difficult to say without investigating it a lot… We're working on a full revamp of the models in 3.0 with a lot less useless foreign keys, which should help a lot. |
Beta Was this translation helpful? Give feedback.
-
|
Just sharing a simple workaround we did to not encounter this error. We noticed that the error is repeatable on events related to |
Beta Was this translation helpful? Give feedback.
-
Hi!
We sometimes experience the following:
This seems to happen when two concurrent requests interfere with each other:
On second try it works.
We already have NGINX rate limiting in place 2r/s but that is not sufficient.
What’s the root cause of this and what’s best way to tackle this?
Beta Was this translation helpful? Give feedback.
All reactions