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

francoisfreitag · 2016-12-13T03:28:06Z

Ticket: https://code.djangoproject.com/ticket/27176

aaugustin · 2016-12-13T08:17:15Z

django/apps/registry.py

@@ -44,7 +44,8 @@ def __init__(self, installed_apps=()):
        self.apps_ready = self.models_ready = self.ready = False

        # Lock for thread-safe population.
-        self._lock = threading.Lock()
+        self._lock = threading.RLock()
+        self._populate_running = False


It's a shame that RLock doesn't expose its _count attribute (in the Python implementation; I didn't check the C implementation but it certainly has a variable that fulfills the same role).

A comment stating that's the reason why you need a separate _populate_running to track whether the lock is acquired for the first time or again by the same thread would help.

See below for a more drastic suggestion to improve the behavior of this flag.

aaugustin · 2016-12-13T08:21:51Z

django/apps/registry.py

-                else:
-                    app_config = AppConfig.create(entry)
-                if app_config.label in self.app_configs:
+            try:


It's unclear to me why you need this try / finally block.

If an exception happens populate() in an "inner" call — and any inner call will raise an exception just below — _populate_running shouldn't be reset to False until the "outer" call terminates.

In fact I don't think we need to reset _populate_running at all. I think it will be just as easy to make it a simple boolean that starts at False and moves to True when populate() is called for the first time. That will be consistent with (apps_|models_|)ready. I'd just call it loading or started and not bother reset it.

I needed to reset the _populate_running flag because I misused set_installed_apps in the test. Using the context manager with self.settings(INSTALLED_APPS=[...]): calls unset_installed_apps and restores the proper installed apps.

aaugustin · 2016-12-13T08:24:41Z

django/apps/registry.py

+
+                # If populate is called concurrently, the code below won't
+                # guarantee that the app_config order matches the order in
+                # INSTALLED_APPS.


I'm... not quite sure what I meant with this comment. Also the problem isn't concurrent calls, it's reentrant calls. The lock prevents concurrent calls when we're reaching this point.

Perhaps it should be rewritten as:

# Prevent reentrant calls to avoid running AppConfig.ready() methods twice.

aaugustin · 2016-12-13T08:25:51Z

django/apps/registry.py

+                # INSTALLED_APPS.
+                if self._populate_running:
+                    raise RuntimeError("populate() isn't reentrant")
+                self._populate_running = True


Non-obviously, there's no race condition here because the RLock only allows one thread to get there.

Consider explaining that in a comment, because it looks like a non-atomic compare-and-set.

timgraham · 2016-12-14T23:27:08Z

tests/apps/tests.py

+            with self.assertRaisesMessage(RuntimeError, "populate() isn't reentrant"):
+                raise exceptions_queue.get(block=True, timeout=3)
+        except Empty:
+            self.assertFalse(True, 'Re-entering populate from the same thread did not raise an exception')


self.fail('Re-entering ...

timgraham · 2016-12-14T23:28:07Z

tests/apps/populate_reentry/models.py

@@ -0,0 +1,2 @@
+# -*- coding: utf-8 -*-


remove # -*- coding: utf-8 -*-?

francoisfreitag · 2016-12-16T01:35:21Z

When test are run in parallel, I cannot spawn another process for the test because multiprocessing.Pool spawns daemon processes, which cannot have children.
I'll look for another solution tomorrow. In the meantime, any idea is very welcome.

aaugustin · 2016-12-16T08:05:05Z

For that kind it used to be acceptable to resort to manual testing, given that it's hard to test initialization code robustly and that such code is rarely modified.

francoisfreitag · 2016-12-18T06:15:49Z

The test does not run when the process executing the test is a daemon (when tests are run in parallel). More details can be found in the second commit message.

I am not sure if it is worth it to include this test. It is easy to run the test using --parallel=1, but Jenkins and most people will likely run the tests in parallel. Is there any similar test case that is executed only when tests are run sequentially?

timgraham · 2017-02-07T01:20:49Z

I don't think there are any tests that are run conditionally based on the number of parallel tests. Anyway, you can rebase your branch and remove usage of six. I'll try to take a closer look later.

francoisfreitag · 2017-02-11T22:07:48Z

I updated the test to use a thread instead of another process, which removes the above limitation (daemonic test runner process unable to spawn an other process to run the test). I can't remember why I discarded this option back then.

timgraham

It might be worth including a mention in the release notes in case anyone hits this when upgrading.

timgraham · 2017-02-16T13:56:48Z

django/apps/registry.py

-            # 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:
+            # A RLock is used to prevent other threads from entering this


used to prevent -> prevents

timgraham · 2017-02-16T13:58:33Z

tests/apps/populate_reentry/models.py

@@ -0,0 +1 @@
+from .user_script import some_value  # NOQA


It could help to add a comment like This import triggers a first/second? call to django.setup().

timgraham · 2017-02-16T14:00:05Z

tests/apps/tests.py

+        Running django.setup() during an ongoing setup() results in a deadlock.
+
+        To prevent Django's tests from hanging indefinitely if a deadlock
+        occurs (ie. the test fails), this test is run in a separate thread.


timgraham · 2017-02-16T14:00:42Z

tests/apps/tests.py

+        """
+        Running django.setup() during an ongoing setup() results in a deadlock.
+
+        To prevent Django's tests from hanging indefinitely if a deadlock


I'm not sure if this is working properly. I reverted django/ and the test seems to hang.

The test relied on mocking threading.RLock. The original code used a Lock, so the mock is ineffective.

Working on this, I remembered why a Thread cannot be used to run the test: it's not possible to kill the test thread when it is stuck. The only (simple) option I can think of is to run the test in another process, using multiprocessing. However, when the tests are executed in parallel, the test processes are daemonic and cannot have children, so it is not possible to start a new process to run the test.

Do you have an idea on how to test this patch? Would it be acceptable to merge it without a corresponding test?

timgraham · 2017-02-16T14:01:47Z

tests/apps/populate_reentry/user_script.py

@@ -0,0 +1,7 @@
+from django import setup
+
+


remove blank line per isort (there's an isort bug that prevents the check from flagging this, but if you run isort manually it'll fix it)

timgraham · 2017-02-16T14:01:59Z

tests/apps/tests.py

@@ -1,4 +1,8 @@
 import os
+import threading
+


remove blank line; same reason as previous comment

timgraham

If Aymeric doesn't think a test is required, I'm okay with it.

timgraham · 2017-02-23T19:10:49Z

docs/releases/2.0.txt

@@ -282,6 +282,9 @@ Miscellaneous
  instead of dotted Python path strings. Django favors callable references
  since they provide better performance and debugging experience.

+* Calling `django.setup()` during a `django.setup()` call (reentrant call)


I guess if the current behavior is the hang forever, then this isn't backwards incompatible considering that it has no chance to break working code. (side note: doouble backticks rather than single are required to have an effect on the rendered docs)

I did not see a good place for it in the release note, so I created a new Apps section. Feel free to change it if there is a better place.

Sorry, I didn't communicate clearly that I don't think a release note is needed. That's not really a "feature" that I'd expect anyone to care about.

Thanks! I'm removing it.

timgraham · 2017-02-23T19:11:29Z

django/apps/registry.py

-            # 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:
+            # A RLock prevents other threads from entering this section. The


aaugustin · 2017-02-23T21:54:39Z

I confirm that I find manual testing acceptable for this change.

Thanks to Aymeric Augustin, Harry Percival and Tim Graham.

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch 4 times, most recently from dcb29d6 to 4baecfa Compare December 13, 2016 05:22

aaugustin reviewed Dec 13, 2016

View reviewed changes

timgraham reviewed Dec 14, 2016

View reviewed changes

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from 4baecfa to 4fa98b6 Compare December 15, 2016 17:02

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch 4 times, most recently from a2f4628 to 68506cb Compare December 18, 2016 06:04

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch 5 times, most recently from da389e3 to ef454c0 Compare December 19, 2016 05:00

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from ef454c0 to cc74c2f Compare January 1, 2017 20:40

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch 6 times, most recently from ea07bd1 to 82b7699 Compare February 11, 2017 05:13

timgraham reviewed Feb 16, 2017

View reviewed changes

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from 82b7699 to acb058f Compare February 18, 2017 23:33

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from acb058f to a923f1c Compare February 18, 2017 23:55

timgraham reviewed Feb 23, 2017

View reviewed changes

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from a923f1c to a97ada2 Compare February 24, 2017 22:51

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

4a12aad

Thanks to Aymeric Augustin, Harry Percival and Tim Graham.

francoisfreitag force-pushed the 27176/error_out_on_setup_reentrant_calls branch from a97ada2 to 4a12aad Compare February 24, 2017 23:40

timgraham merged commit fba4f83 into django:master Feb 25, 2017

francoisfreitag deleted the 27176/error_out_on_setup_reentrant_calls branch February 27, 2017 05:15

blueyed mentioned this pull request Jan 18, 2018

Add testcase to show erroneous multiple runs of django.setup() pytest-dev/pytest-django#538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

francoisfreitag commented Dec 13, 2016

aaugustin Dec 13, 2016

aaugustin Dec 13, 2016

aaugustin Dec 13, 2016

francoisfreitag Dec 15, 2016

aaugustin Dec 13, 2016

aaugustin Dec 13, 2016

timgraham Dec 14, 2016

timgraham Dec 14, 2016

francoisfreitag commented Dec 16, 2016

aaugustin commented Dec 16, 2016

francoisfreitag commented Dec 18, 2016

timgraham commented Feb 7, 2017

francoisfreitag commented Feb 11, 2017

timgraham left a comment

timgraham Feb 16, 2017

timgraham Feb 16, 2017

timgraham Feb 16, 2017

timgraham Feb 16, 2017

francoisfreitag Feb 18, 2017

timgraham Feb 16, 2017

timgraham Feb 16, 2017

timgraham left a comment

timgraham Feb 23, 2017

francoisfreitag Feb 24, 2017

timgraham Feb 24, 2017

francoisfreitag Feb 24, 2017

timgraham Feb 23, 2017

aaugustin commented Feb 23, 2017

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

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

Conversation

francoisfreitag commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoisfreitag commented Dec 16, 2016

aaugustin commented Dec 16, 2016

francoisfreitag commented Dec 18, 2016

timgraham commented Feb 7, 2017

francoisfreitag commented Feb 11, 2017

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaugustin commented Feb 23, 2017