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

Enable persistent connections by default #560

Merged
merged 1 commit into from
Feb 4, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @rattrayalex-stripe

This PR enables persistent connection by default. From my laptop in Europe, running test/flows.spec.js takes 33 seconds with this PR, vs. 1 minute with the master branch.

Tagged as WIP, let me know what you think!

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.

Loving this, so simple.

Let's test:

  • The http-vs-https logic, both in makeRequest and setHttpAgent.
  • That requests reuse the same connection.
  • That you can still override agent (I assume there exists a test for this already).

lib/stripe.js Outdated Show resolved Hide resolved
lib/stripe.js Show resolved Hide resolved
lib/stripe.js Outdated Show resolved Hide resolved
@rattrayalex-stripe
Copy link
Contributor

rattrayalex-stripe commented Feb 1, 2019

Another thought: we should probably add explicit docs to the README mentioning that this is the default, and telling you how to turn it off.

Maybe something like this:


Persistent Connections

As of stripe-node vX.Y.Z, a persistent https connection is kept open by default. This reduces latency when multiple requests are made from the same instance of stripe-node.

If needed, you can disable this behavior by providing your own https agent:

const https = require('https');
stripe.setHttpAgent(new https.Agent());

On the other hand, it might be better not to document this in the README off the bat, so that if users run into problems with it, they open an issue so we can find out about it and fix the problem for everyone.

@ob-stripe ob-stripe force-pushed the ob-persistent-connections branch 2 times, most recently from ce4c190 to 15df51b Compare February 1, 2019 21:09
@ob-stripe ob-stripe force-pushed the ob-persistent-connections branch from 15df51b to d210b7d Compare February 1, 2019 21:10
@ob-stripe
Copy link
Contributor Author

@rattrayalex-stripe I added a test for setHttpAgent, but I'm unsure how to go about testing makeRequest and that connections are actually reused. Any suggestions?

@rattrayalex-stripe
Copy link
Contributor

Eh, yeah, probably not worth it. Whatever we would do there would be pretty heavily dependent on our http mocking library (eg; nock). There isn't much complexity here so I don't see it as a likely candidate for accidental regression.

I think this LGTM!

@ob-stripe ob-stripe changed the title [wip] Enable persistent connections by default Enable persistent connections by default Feb 4, 2019
@ob-stripe
Copy link
Contributor Author

Thanks Alex!

@ob-stripe ob-stripe merged commit c072d87 into master Feb 4, 2019
@ob-stripe ob-stripe deleted the ob-persistent-connections branch February 4, 2019 10:42
@ob-stripe
Copy link
Contributor Author

Released as v6.32.1.

This was referenced Mar 14, 2019
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.

2 participants