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

Multithreading #26

Open
teeberg opened this issue May 2, 2015 · 9 comments
Open

Multithreading #26

teeberg opened this issue May 2, 2015 · 9 comments

Comments

@teeberg
Copy link

teeberg commented May 2, 2015

I'm using a django LiveServerTestCase that runs a server in a second thread. Because the tests and the server run in two different threads now, I can't use gutter switches anymore since the gutter Manager inherits from threading.local. Are there plans for adding support for multiple threads?

Whatever the plans are, it would be nice to mention this limitation more prominently in the README. :-)

@Fluxx
Copy link
Contributor

Fluxx commented May 4, 2015

Hi @teeberg,

Gutter is backed by durabledict, which should give you a consistent[1] view of switches in your data store, even if there is a copy of the manger per thread. Was there a specific issue or some other problem you were running into that was unexpected with multiple threads.

[1] Obviously the memory backend will not be consistent, but the Redis and ZooKeeper ones should be consistent enough no problem.

@teeberg
Copy link
Author

teeberg commented May 4, 2015

Hi @Fluxx, thanks for the fast answer! My understanding of gutter was clearly not good enough to correctly identify the problem. :-) We are in fact using a memory backend in our test environment. When we settled for it, we didn't worry about multiple threads. I also didn't realise this would cause a problem. Thanks for clarifying!

@teeberg teeberg closed this as completed May 4, 2015
@teeberg
Copy link
Author

teeberg commented May 4, 2015

I've debugged some more and tracked down what I think is the real problem: What really doesn't work in the two-thread scenario is the SwitchContextManager from gutter.client.testutils. It temporarily overrides gutter.active, but because the gutter Manager is thread-local, the other thread doesn't pick up this change. Making the Manager inherit from object instead of threading.local fixes the problem, but that can't be the solution. :-)

@teeberg teeberg reopened this May 4, 2015
@Fluxx
Copy link
Contributor

Fluxx commented May 4, 2015

Ah yeah. That's a limitation of SwitchContextManager since, as you mentioned, it monkey patches gutter.active.

IIRC (as I wrote that code over 3 years ago), the intention with making the Manager a threading.local was to prevent people from shooting themselves in the foot by sharing a Manager instance as a singleton between threads and causing problems. I still agree with that sentiment I think, but I do hear you that your use case of the Django LiveServerTestCase would be nice to have thread communication on the Manager state.

I think what might be best in this scenario is to change the class hierarchy a little to have a base somewhat private BaseManager class not inherit from theading.local, and instead make the canonical Manager class inherit from (BaseManager, threading.local). Thus for most users they can just use the canonical Manager class, but for users who need to some more savvy thread coordination they can use a custom manager they inherit from BaseManager.

Thoughts?

@teeberg
Copy link
Author

teeberg commented May 4, 2015

I think it's reasonable to make it thread-local by default. It would of course be nice if the Manager was fully thread-safe but I'm not sure how hard that would be to achieve.

Leaving that out, what you're suggesting sounds like a quick solution that I could work with! It would be awesome if you could make this change here! Maybe other users can benefit from it too, then :-)

@Fluxx
Copy link
Contributor

Fluxx commented May 5, 2015

It would be awesome if you could make this change here! Maybe other users can benefit from it too, then :-)

Yeah I can totally do that. Won't be today though, but by the weekend for sure.

@teeberg
Copy link
Author

teeberg commented May 5, 2015

Awesome, I'm looking forward to that change! Thank you so much for your efforts!

@teeberg
Copy link
Author

teeberg commented Jun 20, 2017

How do you feel about a patch like this one? teeberg@b887062

@thekashifmalik
Copy link

That seems like a good idea. I'll be taking a look at this repo soon.

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

No branches or pull requests

3 participants