From 47d4eedbce345434b57369bb170499c03f556cc7 Mon Sep 17 00:00:00 2001 From: Tim Schilling Date: Mon, 3 Jul 2023 02:13:13 -0500 Subject: [PATCH] Switch StaticFilesPanel to use ContextVar. (#1801) The StaticFilesPanel thread collector was not closing connections to the database. This approach allows those connections to be closed while still collecting context across threads. The test case is to hit an admin login screen with ab -n 200 http://localhost:8000/admin/login/ As far as I can tell, all the static files are collected properly and connections don't stay open. --- debug_toolbar/panels/staticfiles.py | 52 +++++++++++++---------------- debug_toolbar/utils.py | 36 -------------------- docs/changes.rst | 2 ++ 3 files changed, 25 insertions(+), 65 deletions(-) diff --git a/debug_toolbar/panels/staticfiles.py b/debug_toolbar/panels/staticfiles.py index bee336249..5f9efb5c3 100644 --- a/debug_toolbar/panels/staticfiles.py +++ b/debug_toolbar/panels/staticfiles.py @@ -1,3 +1,5 @@ +import contextlib +from contextvars import ContextVar from os.path import join, normpath from django.conf import settings @@ -7,12 +9,6 @@ from django.utils.translation import gettext_lazy as _, ngettext from debug_toolbar import panels -from debug_toolbar.utils import ThreadCollector - -try: - import threading -except ImportError: - threading = None class StaticFile: @@ -33,15 +29,8 @@ def url(self): return storage.staticfiles_storage.url(self.path) -class FileCollector(ThreadCollector): - def collect(self, path, thread=None): - # handle the case of {% static "admin/" %} - if path.endswith("/"): - return - super().collect(StaticFile(path), thread) - - -collector = FileCollector() +# This will collect the StaticFile instances across threads. +used_static_files = ContextVar("djdt_static_used_static_files") class DebugConfiguredStorage(LazyObject): @@ -65,15 +54,16 @@ def _setup(self): configured_storage_cls = get_storage_class(settings.STATICFILES_STORAGE) class DebugStaticFilesStorage(configured_storage_cls): - def __init__(self, collector, *args, **kwargs): - super().__init__(*args, **kwargs) - self.collector = collector - def url(self, path): - self.collector.collect(path) + with contextlib.suppress(LookupError): + # For LookupError: + # The ContextVar wasn't set yet. Since the toolbar wasn't properly + # configured to handle this request, we don't need to capture + # the static file. + used_static_files.get().append(StaticFile(path)) return super().url(path) - self._wrapped = DebugStaticFilesStorage(collector) + self._wrapped = DebugStaticFilesStorage() _original_storage = storage.staticfiles_storage @@ -97,7 +87,7 @@ def title(self): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.num_found = 0 - self._paths = {} + self.used_paths = [] def enable_instrumentation(self): storage.staticfiles_storage = DebugConfiguredStorage() @@ -120,18 +110,22 @@ def nav_subtitle(self): ) % {"num_used": num_used} def process_request(self, request): - collector.clear_collection() - return super().process_request(request) + reset_token = used_static_files.set([]) + response = super().process_request(request) + # Make a copy of the used paths so that when the + # ContextVar is reset, our panel still has the data. + self.used_paths = used_static_files.get().copy() + # Reset the ContextVar to be empty again, removing the reference + # to the list of used files. + used_static_files.reset(reset_token) + return response def generate_stats(self, request, response): - used_paths = collector.get_collection() - self._paths[threading.current_thread()] = used_paths - self.record_stats( { "num_found": self.num_found, - "num_used": len(used_paths), - "staticfiles": used_paths, + "num_used": len(self.used_paths), + "staticfiles": self.used_paths, "staticfiles_apps": self.get_staticfiles_apps(), "staticfiles_dirs": self.get_staticfiles_dirs(), "staticfiles_finders": self.get_staticfiles_finders(), diff --git a/debug_toolbar/utils.py b/debug_toolbar/utils.py index 968160078..7234f1f77 100644 --- a/debug_toolbar/utils.py +++ b/debug_toolbar/utils.py @@ -14,12 +14,6 @@ from debug_toolbar import _stubs as stubs, settings as dt_settings -try: - import threading -except ImportError: - threading = None - - _local_data = Local() @@ -357,33 +351,3 @@ def get_stack_trace(*, skip=0): def clear_stack_trace_caches(): if hasattr(_local_data, "stack_trace_recorder"): del _local_data.stack_trace_recorder - - -class ThreadCollector: - def __init__(self): - if threading is None: - raise NotImplementedError( - "threading module is not available, " - "this panel cannot be used without it" - ) - self.collections = {} # a dictionary that maps threads to collections - - def get_collection(self, thread=None): - """ - Returns a list of collected items for the provided thread, of if none - is provided, returns a list for the current thread. - """ - if thread is None: - thread = threading.current_thread() - if thread not in self.collections: - self.collections[thread] = [] - return self.collections[thread] - - def clear_collection(self, thread=None): - if thread is None: - thread = threading.current_thread() - if thread in self.collections: - del self.collections[thread] - - def collect(self, item, thread=None): - self.get_collection(thread).append(item) diff --git a/docs/changes.rst b/docs/changes.rst index ab69ef99f..cfd950229 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -9,6 +9,8 @@ Pending `__. * Converted cookie keys to lowercase. Fixed the ``samesite`` argument to ``djdt.cookie.set``. +* Converted ``StaticFilesPanel`` to no longer use a thread collector. Instead, + it collects the used static files in a ``ContextVar``. 4.1.0 (2023-05-15) ------------------