Opened 21 months ago

Last modified 5 weeks ago

#28752 assigned Cleanup/optimization

Prevent django.setup() from running multiple times

Reported by: pascal chambon Owned by: pascal chambon
Component: Core (Other) Version: 1.11
Severity: Normal Keywords:
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

I've been bitten numerous times by the impredictable behaviour of django when django.setup() was called numerous times.

In the old days I had exceptions, now it's mainly subtle breakages of logging configuration.

I couldn't find, in the issue tracker or the dev mailing list statements about this subject, others than request from other users encountering the
problem.

For example #26152 concerned script+importable modules.

The latest case in date for me is pytest-django having troubles with these multiple setup() calls : https://github.com/pytest-dev/pytest-django/issues/531 , due to multiple fixtures attempting this auto-setup.

Would it be OK to make django.setup() idempotent, or even expose an "is_ready" flag for easier introspection ?

-- here are some updates, comments get rejected as spam --

Calling django.setup() multiple times is useless, BUT it can happen in lots of cases, that's why imho this case should be handled by the framework to avoid nasty side effects.

These "duplicate calls" often involve the collision between manage.py commands, tests, custom scripts, and external launchers like pytest-django. Plus maybe some corner cases when unittest-style TestCases and pytest-style test functions are mixed in the same project.

Users have to do a real gym to call setup() "at some moment" in all these use cases, yet try to prevent multiple calls of this initialization step (like the if__name__ == "main"' protection). So far my only way out was often to check for (not really undocumented) states of the framework before calling setup().

Change History (21)

comment:1 Changed 21 months ago by Tim Graham

Resolution: duplicate
Status: newclosed
Summary: Django.setup() should be idempotentdjango.setup() should be idempotent

I believe this is addressed in Django 2.0 by #27176. If not, please reopen with a minimal project that reproduces the problem you're encountering.

comment:2 Changed 21 months ago by pascal chambon

Description: modified (diff)
Resolution: duplicate
Status: closednew

comment:3 Changed 21 months ago by Tim Graham

Thanks. Can you explain the use case for calling django.setup() multiple times?

comment:4 Changed 21 months ago by pascal chambon

Description: modified (diff)

comment:5 Changed 21 months ago by pascal chambon

Description: modified (diff)

comment:6 Changed 21 months ago by pascal chambon

Description: modified (diff)

comment:7 Changed 21 months ago by pascal chambon

My comments get marked as spam, I updated the description above.

comment:8 Changed 21 months ago by Tim Graham

Cc: Aymeric Augustin added
Description: modified (diff)

Aymeric, any input?

comment:9 Changed 21 months ago by Aymeric Augustin

django.setup() is already idempotent. However it isn't reentrant — I believe that's what you're actually asking for — and it cannot be made reentrant without breaking its invariants. See #27176 for details.

I don't think we should make changes to Django for accomodating pytest code that does django.setup = lambda: None.

comment:10 Changed 21 months ago by pascal chambon

Summary: django.setup() should be idempotentdjango.setup() should not be runnable multiple times

comment:11 Changed 21 months ago by pascal chambon

OK I think my vocabulary was wrong, it's not (really) an idempotence problem, since django.setup() does more or less the same things on both calls (just skipping apps population phase on the second).

It's not a reentrancy problem, i.e not a problem with multiple threads (or signal interrupts) entering django.setup() concurrently.

It's really just a problem of "multiple successive calls of django.setup()", which are doing silent errors and weird modifications, simply because only the first call of django.setup() makes sense.

Raising an exception on subsequent calls would be a possibility, but it would be a useless hassle, since users just want is to ensure that django was initialized at some point.
That's why I propose we just do a no-op on subsequent calls to django.setup().

(By teh way I don't understand your statement about the "django.setup = lambda: None" snippet - it was just a quick and dirty hack to prevent the multiple runs of django.setup(), which broke the LOGGING config)

comment:12 Changed 21 months ago by Tim Graham

Description: modified (diff)

I don't know. Does that change risk breaking working code where multiple calls to django.setup() has an intended effect?

comment:13 Changed 21 months ago by pascal chambon

Well, I'm usually quite dedicated to retrocompatibility (see django-compat-patcher package), but for once any breakage would be due to a strange misuse of django.setup().

In the code below, we see that django.setup() performs 3 steps :

  • overridding logging configuration with django settings
  • setting the script prefix
  • populating the apps registry

The last 2 steps are now idempotent it seems.

Only overridding logging breaks some setups (eg. with pytest fixtures), and I can't find any use case where it would be a wanted behaviour.
If users want to reset logging several times, they may as well call configure_logging() by themselves.

Until django.setup() is protected against double executions, we'll have weird bugs surfacing each time we add new steps to it (are these idempotent, or reentrant, or runnable multiple times...), so I'd advocate fixing this once and for all.

On "how" to do it, I think a threading lock + a boolean guard would be easy and sufficient, wouldn't they ? With a system to raise an error if django.setup() ends up being called multiple times by the same thread (which often smells like missing "if name == 'main'" conditions in imported scripts).

configure_logging(settings.LOGGING_CONFIG, settings.LOGGING)

if set_prefix:
        set_script_prefix(
            '/' if settings.FORCE_SCRIPT_NAME is None else settings.FORCE_SCRIPT_NAME
        )

apps.populate(settings.INSTALLED_APPS)

comment:14 Changed 19 months ago by Tim Graham

Summary: django.setup() should not be runnable multiple timesPrevent django.setup() from running multiple times
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I'm not completely sure if a change is feasible but I guess we could evaluate a patch.

comment:15 Changed 7 weeks ago by Daniel Hahler

This would be nice to have indeed.

Pascal, do you plan to provide a patch for this?

btw: the following can be used as a workaround (also used by pytest-django):

    import django.apps

    if not django.apps.apps.ready:
        django.setup()

comment:16 Changed 7 weeks ago by pascal chambon

Thanks for reminding me of this ticket, my latest struggles with pytest-django made me think a more global solution to the problem of django setup and complex environments, and this ticket is for me now superseded by https://code.djangoproject.com/ticket/30536 - hoping that the new setting I introduce will be OK too.

comment:17 Changed 7 weeks ago by Carlton Gibson

Let's keep the discussion from #30536 here. (It's ≈the same ticket AFAICS, and the history here is informative.)

PR there is here.

One additional element added there is to make the the setup() code pluggable. (i.e. whilst we make setup() exit if run multiple times, allow user code to be run before/after as well.) A setting is proposed for this. MUST it be a setting…?

comment:18 Changed 7 weeks ago by Carlton Gibson

Patch needs improvement: set

Looking at the PR, there are two things going on there:

  1. Making setup() idempotent. Let's handle that part here.
  2. Allowing specifying custom logic. Let's handle that on #30536.

(The one looks less controversial than the other...)

Pascal, if you could split the PR into two, targeting the one as Fixed #28752 -- ... that would be great.

comment:19 Changed 7 weeks ago by pascal chambon

OK let's handle 1 feature at a time :)

Here is the PR only for idempotence (and django.is_ready flag) : https://github.com/django/django/pull/11440

Last edited 7 weeks ago by pascal chambon (previous) (diff)

comment:20 Changed 6 weeks ago by pascal chambon

The latest version protects setup with a threading lock, so that idempotence gets enforced even in multithreaded lazy-loading contexts.

comment:21 Changed 5 weeks ago by pascal chambon

Owner: changed from nobody to pascal chambon
Status: newassigned

OK a few tweaks and bugfixes later, this PR seems now ready to me B-) -> https://github.com/django/django/pull/11440

Note: See TracTickets for help on using tickets.
Back to Top