Skip to content

Commit

Permalink
Make RequestsClient thread-safe
Browse files Browse the repository at this point in the history
It came up in #526 that our implementation isn't quite thread-safe.
*Most* HTTP clients are by virtue of the fact that their underlying
libraries are, but the recommended client, `RequestsClient` for the
Requests library, creates and reuses only a single session for all the
requests it makes.

The Requests maintainers are non-committal when it comes to the thread
safety of a session, but the general sentiment of its userbase is that
sessions are not thread safe.

Here we tweak the `RequestsClient` implementation so that we create one
session per thread by leveraging Python's thread-local storage.

Unfortunately, this implementation still leaves a pitfall in that if you
explicitly pass in a session in, it will get used between all threads.
The ideal thing to do here I think would be to probably not accept a
session object anymore, but that'd be a breaking change. One possibility
is that we could start warning about the use of the session parameter,
but for the time being I've opted to just leave this as a known problem
(given that hopefully this patch will still be a strict improvement).
  • Loading branch information
brandur committed Jan 29, 2019
1 parent 3abca75 commit 6cf50a6
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions stripe/http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import email
import time
import random
import threading

import stripe
from stripe import error, util, six
Expand Down Expand Up @@ -101,6 +102,8 @@ def __init__(self, verify_ssl_certs=True, proxy=None):
)
self._proxy = proxy.copy() if proxy else None

self._thread_local = threading.local()

def request_with_retries(self, method, url, headers, post_data=None):
num_retries = 0

Expand Down Expand Up @@ -191,7 +194,7 @@ class RequestsClient(HTTPClient):
def __init__(self, timeout=80, session=None, **kwargs):
super(RequestsClient, self).__init__(**kwargs)
self._timeout = timeout
self._session = session or requests.Session()
self._thread_local.session = session or requests.Session()

def request(self, method, url, headers, post_data=None):
kwargs = {}
Expand All @@ -205,7 +208,7 @@ def request(self, method, url, headers, post_data=None):

try:
try:
result = self._session.request(
result = self._thread_local.session.request(
method,
url,
headers=headers,
Expand Down Expand Up @@ -285,8 +288,8 @@ def _handle_request_error(self, e):
raise error.APIConnectionError(msg, should_retry=should_retry)

def close(self):
if self._session is not None:
self._session.close()
if self._thread_local.session is not None:
self._thread_local.session.close()


class UrlFetchClient(HTTPClient):
Expand Down

0 comments on commit 6cf50a6

Please sign in to comment.