Skip to content

Commit

Permalink
Don't try to undo cache method monkey patching (#1770)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
living180 committed May 9, 2023
1 parent a3090b4 commit a6b65a7
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 45 deletions.
82 changes: 37 additions & 45 deletions debug_toolbar/panels/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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")
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand Down
3 changes: 3 additions & 0 deletions docs/changes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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)
------------------
Expand Down

0 comments on commit a6b65a7

Please sign in to comment.