Opened 3 years ago

Closed 2 years ago

#27176 closed Bug (fixed)

django.setup() should raise an exception instead of hanging on re-entrant calls

Reported by: Aymeric Augustin Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords: app-loading
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#25864 and #26152 discuss issues with calling django.setup() in a file that is imported during the app-loading process. This is actually about apps.populate() -- I wrote django.setup() in the title because it's the public API people are familiar with.

Actually there are three cases.

  1. If django.setup() has already completed successfully, calling it again returns immediately.
        if self.ready:
            return
  1. If django.setup() is running in another thread, calling it again returns as soon as that other thread has completed the process.
        # populate() might be called by two threads in parallel on servers
        # that create threads before initializing the WSGI callable.
        with self._lock:
            if self.ready:
                return
  1. If django.setup() is running in the same thread, calling it must crash. Indeed, django.setup() isn't allowed to return until app-loading has completed. The first call cannot complete without going through the second one, which cannot return, so there's a deadlock. The code tries to detect this condition:
            # app_config should be pristine, otherwise the code below won't
            # guarantee that the order matches the order in INSTALLED_APPS.
            if self.app_configs:
                raise RuntimeError("populate() isn't reentrant")

#26152 suggest that the code hangs instead of raising an exception. Indeed, the second call must block on with self._lock:, since self._lock isn't reentrant. Making it a RLock instead of a Lock should allow the second thread to proceed and to hit that exception.

Also the error condition is incorrect: it won't trigger if the reentrant call happens while importing the first app. (We're reasoning in the single threaded case, which is easy.) Instead populate() should set a private attribute before it starts importing apps and raise the exception if that attribute is set. Alternatively it could check if the RLock is being taken for the second time, but no public API for this appears to exist.

Change History (9)

comment:1 Changed 3 years ago by Aymeric Augustin

The ticket description is purely based #26152 and on code analysis. I didn't test my theory.

I'd like to investigate further and submit a patch, but I'm not sure when I'll have time for that, so feel free to assign the ticket and work on it if you're interested.

comment:2 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 2 years ago by François Freitag

Has patch: set

comment:4 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Comments for improvement are on the PR.

comment:5 Changed 2 years ago by François Freitag

Patch needs improvement: unset

comment:6 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:7 Changed 2 years ago by François Freitag

Patch needs improvement: unset

comment:8 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:9 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In fba4f83:

Fixed #27176 -- Raised an exception for reentrant calls to apps.populate().

Thanks to Aymeric Augustin, Harry Percival, and Tim Graham.

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