diff --git a/djblets/extensions/manager.py b/djblets/extensions/manager.py index f5ada06fe447f3fec72da657484e934e3c8d879f..d9be81309070ec84971af83573ceb66ae24e29de 100644 --- a/djblets/extensions/manager.py +++ b/djblets/extensions/manager.py @@ -34,6 +34,7 @@ import pkg_resources import shutil import sys import tempfile +import threading import time import traceback @@ -181,6 +182,7 @@ class ExtensionManager(object): # State synchronization self._sync_key = make_cache_key('extensionmgr:%s:gen' % key) self._last_sync_gen = None + self._load_lock = threading.Lock() self.dynamic_urls = DynamicURLResolver() @@ -398,6 +400,10 @@ class ExtensionManager(object): If full_reload is passed, all state is cleared and we reload all extensions and state from scratch. """ + with self._load_lock: + self._load_extensions(full_reload) + + def _load_extensions(self, full_reload=False): if full_reload: # We're reloading everything, so nuke all the cached copies. self._clear_extensions() diff --git a/djblets/extensions/middleware.py b/djblets/extensions/middleware.py index 7faa82ce74148b79afc488169554633d7c119e51..669a55ee184380dd188620d2da00f5ba9ef73b78 100644 --- a/djblets/extensions/middleware.py +++ b/djblets/extensions/middleware.py @@ -25,11 +25,18 @@ from __future__ import unicode_literals +import threading + from djblets.extensions.manager import get_extension_managers class ExtensionsMiddleware(object): """Middleware to manage extension lifecycles and data.""" + def __init__(self, *args, **kwargs): + super(ExtensionsMiddleware, self).__init__(*args, **kwargs) + + self._lock = threading.Lock() + def process_request(self, request): self._check_expired() @@ -49,7 +56,11 @@ class ExtensionsMiddleware(object): """ for extension_manager in get_extension_managers(): if extension_manager.is_expired(): - extension_manager.load(full_reload=True) + with self._lock: + # Check again, since another thread may have already + # reloaded. + if extension_manager.is_expired(): + extension_manager.load(full_reload=True) class ExtensionsMiddlewareRunner(object): diff --git a/djblets/extensions/tests.py b/djblets/extensions/tests.py index fedda87d44016d06828f4a9f36bdc53356b2008b..4909428280e6c4f47760910329d14cfcbb3fd0a8 100644 --- a/djblets/extensions/tests.py +++ b/djblets/extensions/tests.py @@ -25,7 +25,10 @@ from __future__ import unicode_literals +import logging import os +import threading +import time from django.conf import settings from django.conf.urls import include, patterns @@ -366,7 +369,7 @@ class ExtensionHookPointTest(TestCase): self.assertTrue(self.dummy_hook not in self.extension_hook_class.hooks) -class ExtensionManagerTest(TestCase): +class ExtensionManagerTest(SpyAgency, TestCase): def setUp(self): class TestExtension(Extension): """An empty, dummy extension for testing""" @@ -463,6 +466,89 @@ class ExtensionManagerTest(TestCase): self.assertEqual(len(URLHook.hooks), 0) + def test_load_concurrent_threads(self): + """Testing ExtensionManager.load with concurrent threads""" + # There are a number of things that could go wrong both during + # uninitialization and during initialization of extensions, if + # two threads attempt to reload at the same time and locking isn't + # properly implemented. + # + # Extension uninit could be called twice, resulting in one thread + # attempting to access state that's already been destroyed. We + # could end up hitting: + # + # "Extension's installed app is missing a ref count." + # "'' object has no attribute 'info'." + # + # (Without locking, we end up hitting the latter in this test.) + # + # If an extension is being initialized twice simultaneously, then + # it can hit other errors. An easy one to hit is this assertion: + # + # assert extension_id not in self._extension_instances + # + # With proper locking, these issues don't come up. That's what + # this test case is attempting to check for. + def _sleep_and_call(manager, orig_func, *args): + # This works well enough to throw a monkey wrench into things. + # One thread will be slightly ahead of the other. + time.sleep(0.2) + + try: + orig_func(*args) + except Exception as e: + logging.error('%s\n', e, exc_info=1) + exceptions.append(e) + + def _init_extension(manager, *args): + _sleep_and_call(manager, orig_init_extension, *args) + + def _uninit_extension(manager, *args): + _sleep_and_call(manager, orig_uninit_extension, *args) + + def _loader(main_connection): + # Insert the connection from the main thread, so that we can + # perform lookups. We never write. + from django.db import connections + connections['default'] = main_connection + + self.manager.load(full_reload=True) + + # Enable one extension. This extension's state will get a bit messed + # up if the thread locking fails. We only need one to trigger this. + self.assertEqual(len(self.manager.get_installed_extensions()), 1) + self.manager.enable_extension(self.extension_class.id) + + orig_init_extension = self.manager._init_extension + orig_uninit_extension = self.manager._uninit_extension + + self.spy_on(self.manager._load_extensions) + self.spy_on(self.manager._init_extension, call_fake=_init_extension) + self.spy_on(self.manager._uninit_extension, + call_fake=_uninit_extension) + + # Store the main connection. We're going to let the threads share it. + # This trick courtesy of the Django unit tests + # (django/tests/bakcends/tests.py) + from django.db import connections + main_connection = connections['default'] + main_connection.allow_thread_sharing = True + + exceptions = [] + + # Make the load request twice, simultaneously. + t1 = threading.Thread(target=_loader, args=[main_connection]) + t2 = threading.Thread(target=_loader, args=[main_connection]) + t1.start() + t2.start() + t1.join() + t2.join() + + self.assertEqual(len(self.manager._load_extensions.calls), 2) + self.assertEqual(len(self.manager._uninit_extension.calls), 2) + self.assertEqual(len(self.manager._init_extension.calls), 2) + self.assertEqual(exceptions, []) + def test_enable_registers_static_bundles(self): """Testing ExtensionManager registers static bundles when enabling extension""" settings.PIPELINE_CSS = {}