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

Load helper modules once per Environment instance. #56

Closed
wants to merge 1 commit into from

Conversation

pmclanahan
Copy link

This is my proposal for fixing the threading issues noted
by @peterbe in #55. Tox is happy, but I'm not sure how
well covered an issue with this is. I have also broken backward compat
if jingo.load_helpers is considered public API.

cc @jsocol @jezdez @jbalogh

@jsocol
Copy link
Collaborator

jsocol commented Apr 16, 2014

@peterbe, can you give this one a shot and see if it fixes your issues?

@peterbe
Copy link

peterbe commented Apr 16, 2014

@pmclanahan I like it. Let me have a play.

@jsocol the sad thing is that I'm not able to reproduce the threading problem. Most likely because I'm not setting up my apache on EC2 the same way as Mozilla IT does it.

@pmclanahan
Copy link
Author

All I know is Apache 2.2, MPM-Worker, ModWsgi in daemon mode is what bedrock used when we saw issues. I think the wsgi daemon process was set to 14 threads and 2 procs, though the number of procs shouldn't matter. Set it to a bunch of threads and 1 proc and hit it with a bunch of simultaneous requests using something like bees with machine guns. Might work.

@peterbe peterbe mentioned this pull request Apr 16, 2014
@peterbe
Copy link

peterbe commented Apr 23, 2014

I think the reason why I can't reproduce this on EC2 is because it's hard to install mod_wsgi for python 2.6 and I have too little patience (considering the ROI) to bother.

If I continue to get lots of problems on airmozilla I'm going to ask IT to switch to multiple processes instead of multiple threads.

But any reduction of global is a friend of mine.

@jsocol
Copy link
Collaborator

jsocol commented Jul 3, 2014

I'm not merging this now because we have no reliable info that it actually fixes anything. I realize we might not be able to test it in the test suite (which... ugh, but OK) but we need to have some evidence that it's a helpful switch.

@pmclanahan
Copy link
Author

Agreed. Peter and I were confident we'd get some info from our load testing, but we really didn't. If I find an opportunity to reproduce this and test I'll come back to it. Thanks James.

@jsocol
Copy link
Collaborator

jsocol commented May 8, 2015

I'm going to close this out. Anyone runs into it again, please re-open.

@jsocol jsocol closed this May 8, 2015
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.

None yet

4 participants