deadlock risk from using cache a load time

[John Bazik]

I'm encountering a nested call to apps.populate in 1.8.7 which causes deadlock at load time.  It's called first from django.setup(), then from LocMemCache and the process hangs on a lock.  I'm posting here because I think the problem is principally within django - the apps I'm using are doing reasonable things.

I posted the details to django-users, but the only response I've gotten is a suggestion to ask you folks:

https://groups.google.com/forum/#!topic/django-users/WiRzQ8EFOdo

John

[Florian Apolloner]

On Wednesday, January 13, 2016 at 6:58:25 PM UTC+1, John Bazik wrote:

I'm posting here because I think the problem is principally within django - the apps I'm using are doing reasonable things.

I wouldn't call multisite/hacks.py reasonable :D That said, unpickling models requires apps to be populated (relations etc…), therefor do not attempt to do such an action during population.

Cheers,
Florian

[John Bazik]

I wouldn't call multisite/hacks.py reasonable :D That said, unpickling models requires apps to be populated (relations etc…), therefor do not attempt to do such an action during population.

Thanks.  Shai offered the same opinion and I'm following up with the django-cms developers.

John

[John Bazik]

I got a patch from the django-cms folks that moves template loading into their AppConfig.ready() routine, but the problem persists.

The last few lines of apps.populate are:

            self.models_ready = True

            for app_config in self.get_app_configs():
                app_config.ready()

            self.ready = True

At this point, django has the lock, app_config.ready() invokes the ready code for my apps, and I'm back where I started (nested call to apps.populate and deadlock).

In db/models/base.py, in model_unpickle, I see this:

    if isinstance(model_id, tuple):
        if not apps.ready:
            apps.populate(settings.INSTALLED_APPS)
        model = apps.get_model(*model_id)

Can that test instead be "if not apps.models_ready"?  That would prevent the nested call and fix my problem.

John

[Aymeric Augustin]

On 26 janv. 2016, at 00:43, John Bazik <jba...@gmail.com> wrote:

In db/models/base.py, in model_unpickle, I see this:

    if isinstance(model_id, tuple):
        if not apps.ready:
            apps.populate(settings.INSTALLED_APPS)
        model = apps.get_model(*model_id)

Can that test instead be "if not apps.models_ready"?  That would prevent the nested call and fix my problem.

That would be consistent with the implementation of apps.get_model() which only checks models_ready.

However I think there's a bigger problem with this code. apps.populate() should only be called by django.setup(). I think we should revert 108b8bf852c76855ed98f5abe55db1da845598e7.

External scripts must call django.setup() before using the ORM, whether they’re unpickling models or doing anything else with them. This was a known backwards incompatibility in 1.7: https://docs.djangoproject.com/en/1.9/releases/1.7/#standalone-scripts

-- 

Aymeric.

[Florian Apolloner]

On Tuesday, January 26, 2016 at 9:52:01 AM UTC+1, Aymeric Augustin wrote:

However I think there's a bigger problem with this code. apps.populate() should only be called by django.setup(). I think we should revert 108b8bf852c76855ed98f5abe55db1da845598e7.

Revert yes, but a check like that should stay and maybe raise an error instead of calling apps.populate.

[Aymeric Augustin]

> On 26 janv. 2016, at 09:57, Florian Apolloner <f.apo...@gmail.com> wrote:
>
> Revert yes, but a check like that should stay and maybe raise an error instead of calling apps.populate.

The following line, `apps.get_model(…)`, raises an exception when `not apps.models_ready`.

--
Aymeric.

[Tim Graham]

Oops, here's the pull request: https://github.com/django/django/pull/6044

WIth the fix reverted and the missing django.setup() call, the exception is "django.core.exceptions.AppRegistryNotReady: Models aren't loaded yet.". I don't think we need a different exception in model_unpickle(), but maybe that message could mention a missing call to django.setup() as a common cause? Something like "Models aren't loaded yet. Are you missing django.setup() in a standalone script?".

[Aymeric Augustin]

On 26 janv. 2016, at 14:40, Tim Graham <timog...@gmail.com> wrote:

> Something like "Models aren't loaded yet. Are you missing django.setup() in a standalone script?".

We’ve had this discussion before, cf. 8121860be4b9379d107959290e5748bbaf6de1fe ;-)

The exception message could point to the “Troubleshooting” section of docs/ref/applications.txt. We could extend the explanation of what constitutes a “standalone script” — many devs won’t expect celery / rq to fall in this category.

--
Aymeric.

[Tim Graham]

Oops again... sorry for poor memory about the error message.

Regarding the original issue -- in Claude's use case, he says that calling django.setup() before unpickling is not doable, as an external tool is doing the unpickling of model data. Should we say that unpickling model data from non-Django code is no longer possible?

[Aymeric Augustin]

Le 27 janv. 2016 à 00:59, Tim Graham <timog...@gmail.com> a écrit :

Regarding the original issue -- in Claude's use case, he says that calling django.setup() before unpickling is not doable, as an external tool is doing the unpickling of model data. Should we say that unpickling model data from non-Django code is no longer possible?

There's an inconsistency that bothers me in this case with the current implementation (unless I missed something — I didn't try it).

If the task's arguments include at least one model instance, this will implicitly trigger django.setup() and the code can use the ORM (and logging is set up correctly, etc.)

If the code is refactored to pass the instance id in argument and the task function attempts to load the corresponding instance from the database, it will raise an exception when attempting to import the model.

This inconsistency sounds surprising and hard to diagnose. I'm not convinced special casing the situation where a model in passed in argument is the right choice.

I understand that the most obvious place to call django.setup() is at the top of the task function and at this point arguments have already been unpickled. This explains how we came up with the current code. We didn't realize that "unpickle a model before app loading has completed" could happen in other scenarios.

Regardless, I'm skeptical of this hack. I would expect task runners to provide some sort of initialization hook.

For rq, the documentation explains how to "provide your own worker script" to run initialization code. I believe this is something django-rq could easily abstract.

-- 

Aymeric.