nozzlegear / ShopifySharp

ShopifySharp is a .NET library that helps developers easily authenticate with and manage Shopify stores.
https://nozzlegear.com/shopify-development-handbook
MIT License
742 stars 308 forks source link

Enhanced retry strategies for Leaky Bucket and Retry policies #998

Closed nozzlegear closed 7 months ago

nozzlegear commented 7 months ago

This pull request adds small enhancements to the Leaky Bucket execution policy and the Retry execution policy to make them retry certain common request http request failures. I've based this code off of the retry policy found in Microsoft's Azure.Core SDK (MIT licensed), though it's not quite a one-to-one match since we don't have the concept of an HTTP pipeline in ShopifySharp.

These are the status codes that will be retried if a request fails in the Leaky Bucket and Retry execution policies:

case 408: // Request Timeout
case 429: // Too Many Requests
case 500: // Internal Server Error
case 502: // Bad Gateway
case 503: // Service Unavailable
case 504: // Gateway Timeout

The policy will retry these requests up to three times before throwing a ShopifyHttpException.

Importantly, the default behavior of the Leaky Bucket and Retry policies should remain intact -- they won't start retrying additional responses unless you opt-in to the behavior by using the new constructor overloads on each policy. I wanted to make this change available for testing and feedback right now, so I can gather some feedback on it.

Personally I think it makes sense to have this new behavior be the default, as having a 504 Gateway Timeout or 503 Service Unavailable break something instead of being retried is somewhat unexpected.

nozzlegear commented 7 months ago

@clement911 I'm merging this now so I can get a beta build published to unblock some work I'm doing with a client, but would you mind reviewing these changes and letting me know your thoughts whenever you have spare time? No rush, it doesn't have to be soon!

clement911 commented 7 months ago

I think this is fine. I had a custom policy already doing something similar.