Fix #4 - Handle thread safety for shared clients (Celery fixes)#17
Open
Deadpool2000 wants to merge 9 commits intoopenapi:mainfrom
Open
Fix #4 - Handle thread safety for shared clients (Celery fixes)#17Deadpool2000 wants to merge 9 commits intoopenapi:mainfrom
Deadpool2000 wants to merge 9 commits intoopenapi:mainfrom
Conversation
…retry configurations
…quest with special characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hey @francescobianco ,
this is fix for #4
I’ve been looking into the issue where OauthClient was blowing up when shared across Celery workers, and it’s basically down to how we were handling the internal httpx client. Since we were initializing it once in the constructor, every single worker thread was trying to squeeze through the same connection pool at the same time. That’s why users were seeing those random failures and corrupted data - the internal state just couldn't handle that much concurrent noise.
To fix this without making everyone rewrite their Celery tasks, I moved the internal client over to a property backed by
threading.local(). So now, even if you define the client once at the top of your file, each worker thread will lazily spin up its own private connection the first time it actually needs to make a call. It completely solves the thread-safety headache while keeping the code looking exactly the same from the outside.One thing I wanted to be careful about was not breaking the custom client injection. I know some people pass in their own sessions for custom retries or proxies, so I made sure that if you explicitly pass a
clientin the constructor, it still uses that exact object across all threads. This keeps things backward compatible for power users while making the default behavior safe for everyone else.I also threw in a new test file,
tests/test_thread_safety.py, which just fires off a bunch of threads and makes sure they aren't stepping on each other's toes by checking that they each get a unique client instance. All the original tests pass too, since the mocks still hook into the property correctly.Let me know what you think!