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
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ const stripe = Stripe('sk_test_...');

### Usage with TypeScript

Stripe does not currently maintain typings for this package, but there are
community typings available from DefinitelyTyped.
Stripe does not currently maintain typings for this package, but there are
community typings available from DefinitelyTyped.

To install:

Expand Down Expand Up @@ -145,6 +145,15 @@ if (process.env.http_proxy) {
}
```

### Network retries

Automatic network retries can be enabled with `setMaxNetworkRetries`. This will retry requests `n` times with exponential backoff if they fail due to connection, conflict or rate limiting errors. [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.

```js
// Retry a request once before giving up
stripe.setMaxNetworkRetries(1);
```

### Examining Responses

Some information about the response which generated a resource is available
Expand Down
88 changes: 82 additions & 6 deletions lib/StripeResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
var http = require('http');
var https = require('https');
var path = require('path');
var uuid = require('uuid/v4');

var utils = require('./utils');
var Error = require('./Error');
Expand Down Expand Up @@ -182,7 +183,11 @@ StripeResource.prototype = {
};
},

_errorHandler: function(req, callback) {
_generateConnectionErrorMessage: function(requestRetries) {
return 'An error occurred with our connection to Stripe.' + (requestRetries > 0 ? ' Request was retried ' + requestRetries + ' times.' : '');
},

_errorHandler: function(req, requestRetries, callback) {
var self = this;
return function(error) {
if (req._isAborted) {
Expand All @@ -192,14 +197,50 @@ StripeResource.prototype = {
callback.call(
self,
new Error.StripeConnectionError({
message: 'An error occurred with our connection to Stripe',
message: self._generateConnectionErrorMessage(requestRetries),
paulasjes-stripe marked this conversation as resolved.
Show resolved Hide resolved
detail: error,
}),
null
);
}
},

_shouldRetry: function(res, numRetries) {
// Retry on connection error, if we have retries left.
if (!res) {
return numRetries < this._stripe.getMaxNetworkRetries();
}

// Retry on conflict or rate limit error, if we have retries left.
if (res.statusCode === 409 || res.statusCode === 429) {
return numRetries < this._stripe.getMaxNetworkRetries();

Choose a reason for hiding this comment

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

quick question:
the two errors we encounter most often when calling stripe that would be helped by using retries are

  • StripeAPIError
  • StripeConnectionError

I cant find the http codes for those two, but wondering if this list would cover these two errors. Might be nice to let the client set the statuscodes to retry? i wonder if that'd open up the gates to people starting to retry 400s etc

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe Feb 4, 2019

Choose a reason for hiding this comment

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

Hi @suchitagarwal , thanks, that's very helpful to hear!

As it is, the code will not handle StripeAPIError, which is status code 500, but will handle StripeConnectionError. I'm going to change that to handle 500's too, but only when they are GET's or DELETE's (not POST).

The reason for this is that there isn't really much point to retrying a 500'd POST request with an idempotency key, because our idempotency machinery will simply give you a 500 again.

I don't think we'll open up the logic for when to automatically retry, pretty much based for reasons like you mentioned (retrying a 400 isn't a great idea), but users can always implement retries themselves.

Thoughts?

Choose a reason for hiding this comment

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

makes perfect sense. i think handling 500s for GET/DELETE should cover the cases we encounter most often, and not retrying POSTs makes sense. thanks a lot for the quick turnaround on this

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe Feb 5, 2019

Choose a reason for hiding this comment

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

Awesome, thanks for the feedback!

Speaking of, I'm working on an unrelated feature and looking for some early beta users... if you're willing to try something out, drop me a line? email is myGithubUsername.replace('-', '@') + '.com' 😉

}

return false;
},

_getSleepTimeInMS: function(numRetries) {
rattrayalex-stripe marked this conversation as resolved.
Show resolved Hide resolved
var initialNetworkRetryDelay = this._stripe.getInitialNetworkRetryDelay();
var maxNetworkRetryDelay = this._stripe.getMaxNetworkRetryDelay();

// Apply exponential backoff with initialNetworkRetryDelay on the
// number of numRetries so far as inputs. Do not allow the number to exceed
// maxNetworkRetryDelay.
var sleepSeconds = Math.min(
initialNetworkRetryDelay * Math.pow(numRetries - 1, 2),
maxNetworkRetryDelay
);

// Apply some jitter by randomizing the value in the range of
// (sleepSeconds / 2) to (sleepSeconds).
sleepSeconds *= 0.5 * (1 + Math.random());
paulasjes-stripe marked this conversation as resolved.
Show resolved Hide resolved

// But never sleep less than the base sleep seconds.
sleepSeconds = Math.max(initialNetworkRetryDelay, sleepSeconds);
paulasjes-stripe marked this conversation as resolved.
Show resolved Hide resolved

return sleepSeconds * 1000;
},

_defaultHeaders: function(auth, contentLength, apiVersion) {
var userAgentString = 'Stripe/v1 NodeBindings/' + this._stripe.getConstant('PACKAGE_VERSION');

Expand Down Expand Up @@ -258,7 +299,19 @@ StripeResource.prototype = {
makeRequestWithData(null, utils.stringifyRequestData(data || {}));
}

function makeRequest(apiVersion, headers) {
function retryRequest(requestFn, apiVersion, headers, requestRetries) {
requestRetries += 1;

return setTimeout(
requestFn,
self._getSleepTimeInMS(requestRetries),
apiVersion,
headers,
requestRetries
);
}

function makeRequest(apiVersion, headers, numRetries) {
var timeout = self._stripe.getApiField('timeout');
var isInsecureConnection = self._stripe.getApiField('protocol') == 'http';

Expand All @@ -274,6 +327,14 @@ StripeResource.prototype = {
ciphers: 'DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2:!MD5',
});

// If this is a POST and we allow multiple retries, set a idempotency key if one is not
// already provided.
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
if (method === 'POST' && self._stripe.getMaxNetworkRetries() > 0) {
if (!headers.hasOwnProperty('Idempotency-Key')) {
headers['Idempotency-Key'] = uuid();
}
}

var requestEvent = utils.removeEmpty({
api_version: apiVersion,
account: headers['Stripe-Account'],
Expand All @@ -282,15 +343,31 @@ StripeResource.prototype = {
path: path,
});

var requestRetries = numRetries || 0;

req._requestEvent = requestEvent;

req._requestStart = Date.now();

self._stripe._emitter.emit('request', requestEvent);

req.setTimeout(timeout, self._timeoutHandler(timeout, req, callback));
req.on('response', self._responseHandler(req, callback));
req.on('error', self._errorHandler(req, callback));

req.on('response', function(res) {
paulasjes-stripe marked this conversation as resolved.
Show resolved Hide resolved
if (self._shouldRetry(res, requestRetries)) {
return retryRequest(makeRequest, apiVersion, headers, requestRetries);
} else {
return self._responseHandler(req, callback)(res);
}
});

req.on('error', function(error) {
if (self._shouldRetry(null, requestRetries)) {
return retryRequest(makeRequest, apiVersion, headers, requestRetries);
} else {
return self._errorHandler(req, requestRetries, callback)(error);
}
});

req.on('socket', function(socket) {
if (socket.connecting) {
Expand All @@ -307,7 +384,6 @@ StripeResource.prototype = {
});
}
},

};

module.exports = StripeResource;
24 changes: 24 additions & 0 deletions lib/stripe.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ Stripe.USER_AGENT = {

Stripe.USER_AGENT_SERIALIZED = null;

Stripe.MAX_NETWORK_RETRY_DELAY_SEC = 2;
Stripe.INITIAL_NETWORK_RETRY_DELAY_SEC = 0.5;

var APP_INFO_PROPERTIES = ['name', 'version', 'url', 'partner_id'];

var EventEmitter = require('events').EventEmitter;
Expand Down Expand Up @@ -142,6 +145,7 @@ function Stripe(key, version) {
timeout: Stripe.DEFAULT_TIMEOUT,
agent: null,
dev: false,
maxNetworkRetries: 0,
};

this._prepResources();
Expand Down Expand Up @@ -247,6 +251,26 @@ Stripe.prototype = {
return Stripe[c];
},

getMaxNetworkRetries: function() {
return this.getApiField('maxNetworkRetries');
},

setMaxNetworkRetries: function(maxNetworkRetries) {
if ((maxNetworkRetries && typeof maxNetworkRetries !== 'number') || arguments.length < 1) {
throw new Error('maxNetworkRetries must be a number.');
}

this._setApiField('maxNetworkRetries', maxNetworkRetries);
},

getMaxNetworkRetryDelay: function() {
return this.getConstant('MAX_NETWORK_RETRY_DELAY_SEC');
},

getInitialNetworkRetryDelay: function() {
return this.getConstant('INITIAL_NETWORK_RETRY_DELAY_SEC');
},

// Gets a JSON version of a User-Agent and uses a cached version for a slight
// speed advantage.
getClientUserAgent: function(cb) {
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
"eslint": "^4.19.1",
"eslint-plugin-chai-friendly": "^0.4.0",
"mocha": "~5.0.5",
"nock": "^9.0.0",
"nyc": "^11.3.0"
},
"dependencies": {
"lodash.isplainobject": "^4.0.6",
"qs": "~6.5.1",
"safe-buffer": "^5.1.1"
"safe-buffer": "^5.1.1",
"uuid": "^3.3.2"
},
"license": "MIT",
"scripts": {
Expand Down
Loading