search

gocardless / gocardless-pro-python

GoCardless Pro Python Client
MIT License
37 stars 26 forks source link

idempotency-and-retries branch #12

Closed pegler closed 7 years ago

pegler commented 7 years ago

What is the status of the idempotency-and-retries branch? It looks to be functional and is a feature we'd really like to utilize. We can switch to using the github hosted package, but we'd prefer to use the pypi hosted one.

pegler commented 7 years ago

ping @timrogers

timrogers commented 7 years ago

Hi Matt! Looks like we just need to get this properly released - I'll get it done tomorrow and will ping you once it's live 🚀 On Tue, 16 May 2017 at 20:04, Matt notifications@github.com wrote:

ping @timrogers https://github.com/timrogers

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/gocardless/gocardless-pro-python/issues/12#issuecomment-301882989, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHFplwIwiYpmbHN4TKrNv0mT8JI25VDks5r6fNOgaJpZM4Nc7nJ .

timrogers commented 7 years ago

@pegler It's live on PyPi - enjoy 👌 (Just to warn, there's a chance we might yank it tomorrow and re-push with a major version bump, as in some ways the idempotency behaviour change is non-backwards compatible, if you're being ultra-conservative.)

pegler commented 7 years ago

thanks @timrogers. I was just reviewing the code, and I think there may be a serious issue which may result in duplicate resources being created under rare conditions. It looks like in BaseService. _perform_request, POST requests are retried up to 3 times when an error is encountered. The issue I see is that the Idempotency-Key is generated within ApiClient.post, which means the Idempotency-Key will be different per attempt. So if the first attempt fails with an error, it will be retried with a different Idempotency-Key. I think the generation of the Idempotency-Key needs to be moved to within BaseService. _perform_request to ensure all attempted POST requests use the same key.

timrogers commented 7 years ago

@pegler Uh oh! I can believe that - when I wrote the other libraries I specifically tested that, but it might have been missed in this one when it was reviewed. Double-checking now.

timrogers commented 7 years ago

@pegler Yep, agreed! The architecture of this library is a bit odd compared to the others, as it feels more normal for methods with names like post to delegate to _perform_request, rather than the other way round - I think that's possibly how this got missed. I'll yank the version now and we'll fix it tomorrow - thanks so much for spotting this :heart:

timrogers commented 7 years ago

@pegler I've just pinged you an email 📪