load helpers from monkey

[peterbe]

@jbalogh r?

tox still passes happily. And I tried this out in various ways on mozilla/airmozilla and this patch works. Unfortunately I was not able to reproduce the original problem locally.

The problem is that load_helpers() is called on every Environment.get_template() and Environment.from_string(). I suspect this is done here so it can be done really really late and thus make sure all apps have been installed. Because of the excessive calls a non-thread-safe device is put in place using a global variable. That's premature optimization I think.

The reason why it makes sense to do this from monkey.py is because, in django, jingo.monkey is (at least should be) called from the root urls.py which is only invoked by Django AFTER all apps have successfully been installed.

[jbalogh]

Even with threading, how could the global variable be set to True before things are actually loaded? Doesn't everything have to get loaded by some thread before the variable is flipped?

Putting this in the monkey patch seems sketchy since your helpers won't work now without calling a monkeypatch. I was going to give @jsocol some side-eye for adding a monkeypatch in here but I guess I wrote the first version.

Overall, I'm skeptical of this, but it looks to be equally sketchy to the current load_helpers complications.

[peterbe]

I don't know the internals of wsgi threading in apache but airmozilla is not the first site being hit by this. Clearly what happens is that when mod_wsgi spawns a new thread (due to high demand presumably), it copies everything, including the value of the global variable _helpers_loaded when in fact the necessary import statements have not been executed in this new thread.

I'm struggling to start apache here on my laptop and I think virtualbox is busted so I'm going to try to fire up an AWS box and try to reproduce it there.

Mr Threads himself, although less familar with mod_wsgi, might have some ideas; @twobraids?

[peterbe]

Oh, I'm no fan of monkey patching in general but we could rename monkey.py to necessary_glue_code.py :) At some point Django needs to make friends with jinja.

[jezdez]

I would recommend offering a custom AppConfig class for Django >= 1.7 that loads the "monkey patch" in the available ready method. See https://docs.djangoproject.com/en/dev/ref/applications/#django.apps.AppConfig.ready for some more information.

[jbalogh]

AppConfig sounds pretty great. +1 on doing what you need to do here until you can get to 1.7

[peterbe]

AppConfig looks nice but isn't that outside the problem at hand. At some point the helpers need to be imported. Either at run time, after urlconf is loaded (aka. monkey patch). So, what do we do now?

[jezdez]

It's not, the ready method is guaranteed to be loaded during startup time, not a import time side effect like the urls.py and definitely a place to put code that "configures" the app. Something like this.

[peterbe]

I'm at my knowledge limit here with regards to threading. I setup a version of airmozilla on EC2 running with:

WSGIDaemonProcess airmozilla threads=5 user=ubuntu
WSGIScriptAlias / /home/ubuntu/airmozilla/wsgi/playdoh.wsgi
WSGIProcessGroup airmozilla

I then bombarded the same URL that was causing all the errors as described with high concurrency load and it never failed.

Had it failed, I could better test that my patch works. But not yet able. Anybody else knows how to push modwsgi till it starts to smoke?

[peterbe]

@jezdez neat! Looking forward to it but I can't really upgrade to django 1.7 right now.

[peterbe]

I'm not confident this patch is the best way to do it but the global variable is definitely causing really bizarre errors under heavy load. Clearly there is something fishy around global and things not getting imported.

[pmclanahan]

I think the issue is that the load_helpers function is decoupled from the environment instance. The @register.* decorators load helpers into an Environment instance, so I think the switch to say they're loaded should be on the Environment class. I'll submit a proposal PR as well.

As far as this being the issue I'm still not 100% sure, but I do know that it happens when the app is threaded, and only under load, so it's some kind of race. When it happened @jgmize and I went looking for likely culprits and the use of a module global here seemed a likely candidate. There's a similar issue in tower that I hope we can also address.

[jsocol]

Folks should take a look at #50 / #51, too, which is touching related code and there may be a better solution to both.

monkey_patch_for_helpers--. There are well-described autodiscovery mechanisms that work here, and there's no need to "monkey patch" anything to do it. The monkey module is actually monkey patching Django classes.

@pmclanahan's last comment, about consolidating this in the environment, perhaps making the Library an extension of it, seems like the most promising route of exploration.

[peterbe]

I like @pmclanahan 's solution better https://github.com/jbalogh/jingo/pull/56 because it gets rid of the global and it doesn't rely on the monkey.py

By the way, @pmclanahan and I have been trying hard to make a reproducible case of the threads failing. We've both failed. We think it's because we don't know how to install a mod_wsgi that runs python 2.6 in ubuntu :)