[Django] #27176: django.setup() should raise an exception instead of hanging on re-entrant calls
Django
calls
----------------------------------------+-----------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Keywords: app-loading
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+-----------------------------
#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
}}}
2. 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
}}}
3. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27176>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
calls
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:1>
Django
calls
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:2>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/7682
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:3>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
Comment:
Comments for improvement are on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:4>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:5>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:6>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:7>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:8>
Django
calls
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"fba4f831bc75369c975a95c9b7774e9e89f8a2f9" fba4f83]:
{{{
#!CommitTicketReference repository=""
revision="fba4f831bc75369c975a95c9b7774e9e89f8a2f9"
Fixed #27176 -- Raised an exception for reentrant calls to
apps.populate().
Thanks to Aymeric Augustin, Harry Percival, and Tim Graham.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27176#comment:9>