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

Prevent race condition when using requests_futures and ThreadPoolExecutor #183

Merged
merged 3 commits into from
Apr 19, 2022

Conversation

phodge
Copy link
Contributor

@phodge phodge commented Aug 30, 2021

When using requests_future.Session and a ThreadPoolExecutor there is a race condition where one thread removes the monkeypatched session.get_adapter() method before another thread has finished using it. This results in the losing thread mistakenly using the real requests adapters and likely executing a real http request.

This PR wraps the monkeypatching logic in a threading.Lock to prevent multiple requests racing either other.

Given that other session methods are monkeypatched it's possible there are other race conditions in threading contexts, but making requests_mock completely thread-safe is beyond the scope of this PR.

@ticosax
Copy link

ticosax commented Sep 1, 2021

This PR is working for us. Thanks

@phodge
Copy link
Contributor Author

phodge commented Apr 19, 2022

Hi @jamielennox, is there anything I can do to help get this PR merged into a new official release? For the last 8 months I've been using my own fork on github but this has numerous issues, one of them being that pip takes longer to install this one package from github than installing all of my other dozens of requirements combined. I'd much rather help you improve this package than start a permanent fork.

@jamielennox jamielennox merged commit 73f5cb2 into jamielennox:master Apr 19, 2022
@jamielennox
Copy link
Owner

I'm sorry, this shouldn't have gone so long without a release. I've need this and I'll do a few small fixes and cleanup to do a release this week.

Thanks for the prompt and sticking with it.

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.

3 participants