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

Fix session initialization for multi-threaded environments #534

Merged
merged 2 commits into from
Jan 30, 2019

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Jan 30, 2019

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

Fixes #533.

In #531, we initialized the session in a thread-local store in RequestsClient's constructor. However, a single client instance is initialized for all threads, so only the first thread (that initializes the client) gets a session, and all other threads crash when trying to make a request because self._thread_local.session doesn't exist.

This PR fixes this issue by moving the session initialization in the request() method. A new session is only initialized if one doesn't already exist in the thread-local store. This way, each thread gets a session the first time it tries to make a request.

We should probably have an automated test for this, but in the meantime I've tested this with a trivial script:

import threading

import stripe


stripe.api_key = "sk_test_..."

def worker():
    print(stripe.Account.retrieve().id)
    return

threads = []
for i in range(5):
    t = threading.Thread(target=worker)
    threads.append(t)
    t.start()

@ob-stripe ob-stripe force-pushed the ob-fix-thread-local-session branch from d3ab396 to 5acc9d9 Compare January 30, 2019 17:28
@jameshageman-stripe
Copy link
Contributor

@ob-stripe I noticed this error after rebasing #530, and there's a test in there that is failing.

@brandur-stripe
Copy link
Contributor

Oh man, that was really stupid of me. Thanks for the quick fix OB! It must be late there so LMK if you want me to take this over.

@ob-stripe
Copy link
Contributor Author

@brandur-stripe Yeah, would be great if you could pick this one up, thanks! Also take a look at the multithread tests James pushed in #535.

@brandur-stripe
Copy link
Contributor

@ob-stripe Sounds good! Just pushed a commit into your branch.

@brandur-stripe brandur-stripe merged commit aad2c39 into master Jan 30, 2019
@brandur-stripe brandur-stripe deleted the ob-fix-thread-local-session branch January 30, 2019 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'_thread._local' object has no attribute 'session'
4 participants