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

Network retries #559

Merged
merged 21 commits into from
Feb 14, 2019
Merged

Conversation

paulasjes-stripe
Copy link
Contributor

r? @rattrayalex-stripe
cc @stripe/api-libraries

Fixes #558

Adds network retry functionality. The default retry amount is 0 but can be changed via stripe.setMaxNetworkRetries(n). I modeled this after a similar change in stripe-php: https://github.com/stripe/stripe-php/pull/428/files

In the case of a POST request, an idempotency key is generated if not already provided.

This also adds the uuid and nock libraries. The former is for generating idempotency keys, there already is a similar function used in testing however according to our docs [0] uuid/v4 should ideally be used for real requests. I use nock to mock API responses and failures.

[0]: https://stripe.com/docs/api/idempotent_requests

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick high level comments

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stopping review for now, thought I'd share these comments

ptal @paulasjes-stripe

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, have a basket of nits! I still have to review tests, but won't be able to get to that right away.

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Few further nits on tests etc, pretty sure this should be ready to ship thereafter!

ptal @paulasjes-stripe

});

describe('_getSleepTimeInMS', function() {
it('should not exceed the maximum or minimum values', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call me a cranky fellow, but having randomness/fuzzing in tests like this makes me uncomfortable (even though I expect it to pass in all cases). Imagine we had an exceedingly rare edge-case bug, such as failing when Math.random() produces exactly 0.0 - it'd occasionally fail in CI, which is frustrating. Not the end of the world in stripe-node (we can just hit rebuild, and it doesn't block other things from getting released).

To be honest, I'm not sure this test is that important; thoughts on just deleting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I think it's still useful to ensure that the sleep time is a value we'd expect.

I'd prefer a test that very rarely fails over a edge-case bug over never finding the edge-case bug at all. Especially since the Math.random() call is in our production code and not in the test. Also it's nice to have more code coverage.

@paulasjes-stripe
Copy link
Contributor Author

@ob-stripe Done! We now mock _getSleepTimeInMS so the new tests are pretty much instant.

@ob-stripe
Copy link
Contributor

We now mock _getSleepTimeInMS so the new tests are pretty much instant.

Nice! Thanks @paulasjes-stripe.

Reassigning to @brandur-stripe and @rattrayalex-stripe for the final decision regarding the exact retry conditions, but this looks nearly ready to 🚢

@brandur-stripe
Copy link
Contributor

Okay, Alex and I had a long talk over lunch following a long talk yesterday, and we've decided to not retry on 429s for the time being, but allowing for a system that eventually does pending a few server-side improvements that we'd like to make.

I just pushed a commit to take 429s out of the retry equation.

r? @rattrayalex-stripe Mind taking a quick look at that? Thanks!

@brandur-stripe
Copy link
Contributor

(Also, just wanted to note that I'm really happy to see this come into stripe-node. Thanks for the implementation @paulasjes-stripe!)

@remi-stripe
Copy link
Contributor

@rattrayalex-stripe re-assigning to you to release!

@ob-stripe
Copy link
Contributor

Can we squash the commits before merging please? :)

@rattrayalex-stripe
Copy link
Contributor

Thanks for the reminder - will squash and merge before releasing sometime later today.

@rattrayalex-stripe rattrayalex-stripe merged commit de7a7b7 into master Feb 14, 2019
@ob-stripe ob-stripe deleted the paulasjes/issue-558/network-retries branch February 15, 2019 11:26
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* Added constants and tests

* added _shouldRetry and rejigged nock logic

* retry logic

* stop breaking all the other tests

* add idempotency feature for retries

* refactor wip

* put some things back

* yikes

* add test

* quick change

* addressed feedback and switched to a lower nock version that does not have ES6

* addressed more feedback

* removed unused require

* correctly set the idempotency key

* renamed some functions and added better comments

* Added new idempotency test and addressed some feedback

* Updated readme

* Update README.md

* Add 500 and 503 handling

* Speed up network retry tests

* For the time being, don't retry on 429
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* Added constants and tests

* added _shouldRetry and rejigged nock logic

* retry logic

* stop breaking all the other tests

* add idempotency feature for retries

* refactor wip

* put some things back

* yikes

* add test

* quick change

* addressed feedback and switched to a lower nock version that does not have ES6

* addressed more feedback

* removed unused require

* correctly set the idempotency key

* renamed some functions and added better comments

* Added new idempotency test and addressed some feedback

* Updated readme

* Update README.md

* Add 500 and 503 handling

* Speed up network retry tests

* For the time being, don't retry on 429
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.

10 participants