From 18fd55deb4371b3739426b3f59009d56b617d1c4 Mon Sep 17 00:00:00 2001 From: Daniel Harding Date: Mon, 8 May 2023 21:07:23 +0300 Subject: [PATCH] Don't try to undo cache method monkey patching Trying to undo the monkey patch of cache methods in the CachePanel.disable_instrumentation() method is fragile in the presence of other code which may also monkey patch the same methods (such as Sentry's Django integration), and there are theoretically situations where it is actually impossible to do correctly. Thus once a cache has been monkey-patched, leave it that way, and instead rely on checking in the patched methods to see if recording needs to happen. This is done via a _djdt_panel attribute which is set to the current panel in the enable_instrumentation() method and then set to None in the disable_instrumentation() method. --- debug_toolbar/panels/cache.py | 82 ++++++++++++++++------------------- docs/changes.rst | 3 ++ 2 files changed, 40 insertions(+), 45 deletions(-) diff --git a/debug_toolbar/panels/cache.py b/debug_toolbar/panels/cache.py index f5ceea513..69a899ea1 100644 --- a/debug_toolbar/panels/cache.py +++ b/debug_toolbar/panels/cache.py @@ -30,6 +30,27 @@ ] +def _monkey_patch_method(cache, name): + original_method = getattr(cache, name) + + @functools.wraps(original_method) + def wrapper(*args, **kwargs): + panel = cache._djdt_panel + if panel is None: + return original_method(*args, **kwargs) + else: + return panel._record_call(cache, name, original_method, args, kwargs) + + setattr(cache, name, wrapper) + + +def _monkey_patch_cache(cache): + if not hasattr(cache, "_djdt_patched"): + for name in WRAPPED_CACHE_METHODS: + _monkey_patch_method(cache, name) + cache._djdt_patched = True + + class CachePanel(Panel): """ Panel that displays the cache statistics. @@ -72,7 +93,8 @@ def wrapper(self, alias): cache = original_method(self, alias) panel = cls.current_instance() if panel is not None: - panel._monkey_patch_cache(cache) + _monkey_patch_cache(cache) + cache._djdt_panel = panel return cache CacheHandler.create_connection = wrapper @@ -120,14 +142,17 @@ def _store_call_info( def _record_call(self, cache, name, original_method, args, kwargs): # Some cache backends implement certain cache methods in terms of other cache # methods (e.g. get_or_set() in terms of get() and add()). In order to only - # record the calls made directly by the user code, set the _djdt_recording flag - # here to cause the monkey patched cache methods to skip recording additional - # calls made during the course of this call. - cache._djdt_recording = True - t = time.time() - value = original_method(*args, **kwargs) - t = time.time() - t - cache._djdt_recording = False + # record the calls made directly by the user code, set the cache's _djdt_panel + # attribute to None before invoking the original method, which will cause the + # monkey-patched cache methods to skip recording additional calls made during + # the course of this call, and then reset it back afterward. + cache._djdt_panel = None + try: + t = time.time() + value = original_method(*args, **kwargs) + t = time.time() - t + finally: + cache._djdt_panel = self self._store_call_info( name=name, @@ -141,40 +166,6 @@ def _record_call(self, cache, name, original_method, args, kwargs): ) return value - def _monkey_patch_method(self, cache, name): - original_method = getattr(cache, name) - - @functools.wraps(original_method) - def wrapper(*args, **kwargs): - # If this call is being made as part of the implementation of another cache - # method, don't record it. - if cache._djdt_recording: - return original_method(*args, **kwargs) - else: - return self._record_call(cache, name, original_method, args, kwargs) - - wrapper._djdt_wrapped = original_method - setattr(cache, name, wrapper) - - def _monkey_patch_cache(self, cache): - if not hasattr(cache, "_djdt_patched"): - for name in WRAPPED_CACHE_METHODS: - self._monkey_patch_method(cache, name) - cache._djdt_patched = True - cache._djdt_recording = False - - @staticmethod - def _unmonkey_patch_cache(cache): - if hasattr(cache, "_djdt_patched"): - for name in WRAPPED_CACHE_METHODS: - original_method = getattr(cache, name)._djdt_wrapped - if original_method.__func__ == getattr(cache.__class__, name): - delattr(cache, name) - else: - setattr(cache, name, original_method) - del cache._djdt_patched - del cache._djdt_recording - # Implement the Panel API nav_title = _("Cache") @@ -204,7 +195,8 @@ def enable_instrumentation(self): # the .ready() method will ensure that any new cache connections that get opened # during this request will also be monkey patched. for cache in caches.all(initialized_only=True): - self._monkey_patch_cache(cache) + _monkey_patch_cache(cache) + cache._djdt_panel = self # Mark this panel instance as the current one for the active thread/async task # context. This will be used by the CacheHander.create_connection() monkey # patch. @@ -214,7 +206,7 @@ def disable_instrumentation(self): if hasattr(self._context_locals, "current_instance"): del self._context_locals.current_instance for cache in caches.all(initialized_only=True): - self._unmonkey_patch_cache(cache) + cache._djdt_panel = None def generate_stats(self, request, response): self.record_stats( diff --git a/docs/changes.rst b/docs/changes.rst index 014233997..6844b5ce8 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -8,6 +8,9 @@ Pending indentation of ``CASE`` statements and stopped simplifying ``.count()`` queries. * Added support for the new STORAGES setting in Django 4.2 for static files. +* Reworked the cache panel instrumentation code to no longer attempt to undo + monkey patching of cache methods, as that turned out to be fragile in the + presence of other code which also monkey patches those methods. 4.0.0 (2023-04-03) ------------------