Skip to content

Commit a6b65a7

Browse files
authored
Don't try to undo cache method monkey patching (#1770)
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.
1 parent a3090b4 commit a6b65a7

File tree

2 files changed

+40
-45
lines changed

2 files changed

+40
-45
lines changed

debug_toolbar/panels/cache.py

Lines changed: 37 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,27 @@
3030
]
3131

3232

33+
def _monkey_patch_method(cache, name):
34+
original_method = getattr(cache, name)
35+
36+
@functools.wraps(original_method)
37+
def wrapper(*args, **kwargs):
38+
panel = cache._djdt_panel
39+
if panel is None:
40+
return original_method(*args, **kwargs)
41+
else:
42+
return panel._record_call(cache, name, original_method, args, kwargs)
43+
44+
setattr(cache, name, wrapper)
45+
46+
47+
def _monkey_patch_cache(cache):
48+
if not hasattr(cache, "_djdt_patched"):
49+
for name in WRAPPED_CACHE_METHODS:
50+
_monkey_patch_method(cache, name)
51+
cache._djdt_patched = True
52+
53+
3354
class CachePanel(Panel):
3455
"""
3556
Panel that displays the cache statistics.
@@ -72,7 +93,8 @@ def wrapper(self, alias):
7293
cache = original_method(self, alias)
7394
panel = cls.current_instance()
7495
if panel is not None:
75-
panel._monkey_patch_cache(cache)
96+
_monkey_patch_cache(cache)
97+
cache._djdt_panel = panel
7698
return cache
7799

78100
CacheHandler.create_connection = wrapper
@@ -120,14 +142,17 @@ def _store_call_info(
120142
def _record_call(self, cache, name, original_method, args, kwargs):
121143
# Some cache backends implement certain cache methods in terms of other cache
122144
# methods (e.g. get_or_set() in terms of get() and add()). In order to only
123-
# record the calls made directly by the user code, set the _djdt_recording flag
124-
# here to cause the monkey patched cache methods to skip recording additional
125-
# calls made during the course of this call.
126-
cache._djdt_recording = True
127-
t = time.time()
128-
value = original_method(*args, **kwargs)
129-
t = time.time() - t
130-
cache._djdt_recording = False
145+
# record the calls made directly by the user code, set the cache's _djdt_panel
146+
# attribute to None before invoking the original method, which will cause the
147+
# monkey-patched cache methods to skip recording additional calls made during
148+
# the course of this call, and then reset it back afterward.
149+
cache._djdt_panel = None
150+
try:
151+
t = time.time()
152+
value = original_method(*args, **kwargs)
153+
t = time.time() - t
154+
finally:
155+
cache._djdt_panel = self
131156

132157
self._store_call_info(
133158
name=name,
@@ -141,40 +166,6 @@ def _record_call(self, cache, name, original_method, args, kwargs):
141166
)
142167
return value
143168

144-
def _monkey_patch_method(self, cache, name):
145-
original_method = getattr(cache, name)
146-
147-
@functools.wraps(original_method)
148-
def wrapper(*args, **kwargs):
149-
# If this call is being made as part of the implementation of another cache
150-
# method, don't record it.
151-
if cache._djdt_recording:
152-
return original_method(*args, **kwargs)
153-
else:
154-
return self._record_call(cache, name, original_method, args, kwargs)
155-
156-
wrapper._djdt_wrapped = original_method
157-
setattr(cache, name, wrapper)
158-
159-
def _monkey_patch_cache(self, cache):
160-
if not hasattr(cache, "_djdt_patched"):
161-
for name in WRAPPED_CACHE_METHODS:
162-
self._monkey_patch_method(cache, name)
163-
cache._djdt_patched = True
164-
cache._djdt_recording = False
165-
166-
@staticmethod
167-
def _unmonkey_patch_cache(cache):
168-
if hasattr(cache, "_djdt_patched"):
169-
for name in WRAPPED_CACHE_METHODS:
170-
original_method = getattr(cache, name)._djdt_wrapped
171-
if original_method.__func__ == getattr(cache.__class__, name):
172-
delattr(cache, name)
173-
else:
174-
setattr(cache, name, original_method)
175-
del cache._djdt_patched
176-
del cache._djdt_recording
177-
178169
# Implement the Panel API
179170

180171
nav_title = _("Cache")
@@ -204,7 +195,8 @@ def enable_instrumentation(self):
204195
# the .ready() method will ensure that any new cache connections that get opened
205196
# during this request will also be monkey patched.
206197
for cache in caches.all(initialized_only=True):
207-
self._monkey_patch_cache(cache)
198+
_monkey_patch_cache(cache)
199+
cache._djdt_panel = self
208200
# Mark this panel instance as the current one for the active thread/async task
209201
# context. This will be used by the CacheHander.create_connection() monkey
210202
# patch.
@@ -214,7 +206,7 @@ def disable_instrumentation(self):
214206
if hasattr(self._context_locals, "current_instance"):
215207
del self._context_locals.current_instance
216208
for cache in caches.all(initialized_only=True):
217-
self._unmonkey_patch_cache(cache)
209+
cache._djdt_panel = None
218210

219211
def generate_stats(self, request, response):
220212
self.record_stats(

docs/changes.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ Pending
88
indentation of ``CASE`` statements and stopped simplifying ``.count()``
99
queries.
1010
* Added support for the new STORAGES setting in Django 4.2 for static files.
11+
* Reworked the cache panel instrumentation code to no longer attempt to undo
12+
monkey patching of cache methods, as that turned out to be fragile in the
13+
presence of other code which also monkey patches those methods.
1114

1215
4.0.0 (2023-04-03)
1316
------------------

0 commit comments

Comments
 (0)