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 #550

Merged
merged 1 commit into from
Nov 15, 2018
Merged

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Nov 14, 2018

r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries

Instead of creating and closing a new curl handle for each request, create a curl handle once and reuse it for all requests. This should offer a significant performance improvement, since curl will reuse the connection.

Unfortunately, it's difficult to write automated tests for this given the current shape of CurlClient (not really possible to test private static methods). I've ran some manual tests though, and things look pretty good.

This should also be thread-safe. From https://secure.php.net/manual/en/intro.pthreads.php:

Static Members: When a new context is created ( Thread or Worker ), they are generally copied, but resources and objects with internal state are nullified (for safety reasons).

Because CurlClient is instantiated as a singleton via CurlClient::instance(), each thread gets its own client instance (and thus its own curl handle). I've verified that running multiple threads in parallel doesn't cause issues when persistent connections are enabled.

@ob-stripe ob-stripe changed the base branch from master to integration-v7 November 15, 2018 16:02
@ob-stripe ob-stripe changed the title [WIP] Reuse curl handle Enable persistent connections Nov 15, 2018
@ob-stripe ob-stripe mentioned this pull request Nov 15, 2018
26 tasks
@brandur-stripe
Copy link
Contributor

I don't 100% understand the static thread safety thing — it kind of sounds like we're sort of hoping that a cURL connection is correctly identified as stateful and therefore copied safely. That said, if testing yielded good results I guess it's fine.

LGTM.

@ob-stripe
Copy link
Contributor Author

it kind of sounds like we're sort of hoping that a cURL connection is correctly identified as stateful and therefore copied safely.

More precisely, it's the static CurlClient instance that's identified as stateful and nullified, so the first time CurlClient::instance() is called in a new thread, a new CurlClient instance will be created.

@ob-stripe ob-stripe merged commit 6a10e94 into integration-v7 Nov 15, 2018
@ob-stripe ob-stripe deleted the ob-reuse-connection branch November 15, 2018 17:30
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.

3 participants