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

Add an HttpClient interface and NodeHttpClient implementation. #1218

Merged
merged 10 commits into from
Aug 13, 2021
318 changes: 153 additions & 165 deletions lib/StripeResource.js

Large diffs are not rendered by default.

60 changes: 60 additions & 0 deletions lib/net/HttpClient.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'use strict';

/* eslint-disable class-methods-use-this */

/**
* Encapsulates the logic for issuing a request to the Stripe API. This is an
* experimental interface and is not yet stable.
*/
class HttpClient {
makeRequest(
host,
port,
path,
method,
headers,
requestData,
protocol,
timeout
) {
throw new Error('makeRequest not implemented.');
}

/** Helper to make a consistent timeout error across implementations. */
static makeTimeoutError() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great

const timeoutErr = new TypeError(HttpClient.TIMEOUT_ERROR_CODE);
timeoutErr.code = HttpClient.TIMEOUT_ERROR_CODE;
return timeoutErr;
}
}

HttpClient.TIMEOUT_ERROR_CODE = 'ETIMEDOUT';

class HttpClientResponse {
constructor(statusCode, headers) {
this._statusCode = statusCode;
this._headers = headers;
}

getStatusCode() {
return this._statusCode;
}

getHeaders() {
return this._headers;
}

getRawResponse() {
throw new Error('getRawResponse not implemented.');
}

toStream(streamCompleteCallback) {
throw new Error('toStream not implemented.');
}

toJSON() {
throw new Error('toJSON not implemented.');
}
}

module.exports = {HttpClient, HttpClientResponse};
120 changes: 120 additions & 0 deletions lib/net/NodeHttpClient.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
'use strict';

const http = require('http');
const https = require('https');

const {HttpClient, HttpClientResponse} = require('./HttpClient');

const defaultHttpAgent = new http.Agent({keepAlive: true});
const defaultHttpsAgent = new https.Agent({keepAlive: true});
Comment on lines +8 to +9

Choose a reason for hiding this comment

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

Is there a reason not to move this into the constructor? It'd be nice to not have side effects on import in the case that this is being imported in a non-node environment.

Copy link
Contributor

@richardm-stripe richardm-stripe Aug 12, 2021

Choose a reason for hiding this comment

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

We do need a singleton -- if the user instantiates multiple stripe clients (without overriding the agent) then they should all share the same connection pool. We could lazily instantiate it when the constructor is called, though. Although, maybe we should instead ensure that this file is not required in non-node environments? It is NodeHttpClient after all.

Choose a reason for hiding this comment

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

I think this is resolved by @dcr-stripe 's latest changes


/**
* HTTP client which uses the Node `http` and `https` packages to issue
* requests.`
*/
class NodeHttpClient extends HttpClient {
constructor(agent) {
super();
this._agent = agent;
}

makeRequest(
host,
port,
path,
method,
headers,
requestData,
protocol,
timeout
) {
const isInsecureConnection = protocol === 'http';

let agent = this._agent;
if (!agent) {
agent = isInsecureConnection ? defaultHttpAgent : defaultHttpsAgent;
}

const requestPromise = new Promise((resolve, reject) => {
const req = (isInsecureConnection ? http : https).request({
host: host,
port: port,
path,
method,
agent,
headers,
ciphers: 'DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2:!MD5',
});

req.setTimeout(timeout, () => {
req.destroy(HttpClient.makeTimeoutError());
});

req.on('response', (res) => {
resolve(new NodeHttpClientResponse(res));
});

req.on('error', (error) => {
reject(error);
});

req.once('socket', (socket) => {
if (socket.connecting) {
socket.once(
isInsecureConnection ? 'connect' : 'secureConnect',
() => {
// Send payload; we're safe:
req.write(requestData);
req.end();
}
);
} else {
// we're already connected
req.write(requestData);
req.end();
}
});
});

return requestPromise;
}
}

class NodeHttpClientResponse extends HttpClientResponse {
constructor(res) {
super(res.statusCode, res.headers || {});
this._res = res;
}

getRawResponse() {
return this._res;
}

toStream(streamCompleteCallback) {
// The raw response is itself the stream, so we just return that. To be
// backwards comaptible, we should invoke the streamCompleteCallback only
// once the stream has been fully consumed.
this._res.once('end', () => streamCompleteCallback());
return this._res;
}

toJSON() {
return new Promise((resolve, reject) => {
let response = '';

this._res.setEncoding('utf8');
this._res.on('data', (chunk) => {
response += chunk;
});
this._res.once('end', () => {
try {
resolve(JSON.parse(response));
} catch (e) {
reject(e);
}
});
});
}
}

module.exports = {NodeHttpClient, NodeHttpClientResponse};
15 changes: 14 additions & 1 deletion lib/stripe.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const ALLOWED_CONFIG_PROPERTIES = [
'typescript',
'maxNetworkRetries',
'httpAgent',
'httpClient',
'timeout',
'host',
'port',
Expand All @@ -49,6 +50,10 @@ const {emitWarning} = utils;
Stripe.StripeResource = require('./StripeResource');
Stripe.resources = resources;

const {HttpClient, HttpClientResponse} = require('./net/HttpClient');
Stripe.HttpClient = HttpClient;
Stripe.HttpClientResponse = HttpClientResponse;

function Stripe(key, config = {}) {
if (!(this instanceof Stripe)) {
return new Stripe(key, config);
Expand Down Expand Up @@ -79,6 +84,8 @@ function Stripe(key, config = {}) {
);
}

const agent = props.httpAgent || null;

this._api = {
auth: null,
host: props.host || DEFAULT_HOST,
Expand All @@ -92,7 +99,8 @@ function Stripe(key, config = {}) {
props.maxNetworkRetries,
0
),
agent: props.httpAgent || null,
agent: agent,
httpClient: props.httpClient || Stripe.createNodeHttpClient(agent),
dev: false,
stripeAccount: props.stripeAccount || null,
};
Expand Down Expand Up @@ -126,6 +134,11 @@ function Stripe(key, config = {}) {
Stripe.errors = require('./Error');
Stripe.webhooks = require('./Webhooks');

Stripe.createNodeHttpClient = (agent) => {
const {NodeHttpClient} = require('./net/NodeHttpClient');
return new NodeHttpClient(agent);
};

Stripe.prototype = {
/**
* @deprecated will be removed in a future major version. Use the config object instead:
Expand Down
Loading