Cache panel work #1626
Merged
Cache panel work #1626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
FYI, I think this should also fix #799 (though that was not my goal when I started working on these changes). |
living180
force-pushed
the
cache_panel_work
branch
2 times, most recently
from
7eee842 to
9eff0ac
Compare
matthiask
approved these changes
May 23, 2022
debug_toolbar/signals.py
Outdated
| from django.dispatch import Signal | ||
|
|
||
| # Signal used internally for testing | ||
| toolbar_created = Signal() |
There was a problem hiding this comment.
I know it's not documented but I'm a little bit uneasy; this signal instance sounds or looks a bit too public to me.
Maybe make this _toolbar_created, and/or maybe even move this to the debug_toolbar/toolbar.py module to lower its visibility?
There was a problem hiding this comment.
I've made it a class attribute on the DebugToolbar class under the name _created. Hopefully that will lower its profile?
Will be used for asgiref.local.Local, a context-aware threadlocal.local replacement also used internally by Django. Depend on the same version required by the earliest supported version of Django (3.2).
Add a mechanism for integration tests to access the toolbar used for a particular request: Update DebugToolbar to emit a signal on creation referencing itself. Then create and use a custom Django test client that connects to this signal to capture the toolbar that was created while the request was being processed, and to store the toolbar on the response for use by tests.
For integration tests using the cache panel, check the panel associated with the toolbar created for the request, rather than the toolbar created for the test case.
living180
force-pushed
the
cache_panel_work
branch
from
9eff0ac to
16efe2a
Compare
This method will be called for all installed panels from the DebugToolbarConfig.ready() method during Django initialization to support initialization that needs to happen unconditionally for the panel regardless of whether it is enabled for a particular request.
Using signals for monitoring cache calls gives incorrect results in the face of concurrency. If two requests are being processed concurrently in different threads, they will store each other's cache calls because both panels will be subscribed to the same signal. Additionally, rework the enable_instrumentation() mechanism to monkey patch methods on the cache instances directly instead of replacing the cache instances with wrapper classes. This should eliminate the corner cases mentioned in the (now-removed) disable_instrumentation() comments.
living180
force-pushed
the
cache_panel_work
branch
from
16efe2a to
4b77ec7
Compare
matthiask
added a commit
that referenced
this pull request
May 23, 2022
Depending on asgiref via Django is fine. Refs #1626.
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR re-architects
CachePanelcall recording. Using signals for monitoring cache calls gives incorrect results in the face of concurrency. If two requests are being processed concurrently in different threads, they will store each other's cache calls because both panels will be subscribed to the same signal. This PR updates the code to record the cache calls directly via a method on the panel instance rather than going through signals.Additionally, it reworks the
enable_instrumentation()mechanism to monkey patch methods on the cache instances directly instead of replacing the cache instances with wrapper classes. This should eliminate the corner cases mentioned in the (now-removed)disable_instrumentation()comments.One change I made as part of the preparatory work for these changes is adding a
ready()class method to the panel API, to be called from theDebugToolbarConfig.ready()method. This provides a clean way for panels to perform any initialization or instrumentation that needs to be done unconditionally for the panel regardless of whether it is enabled for a particular request. This in theory could cause backwards compatibility issues if a third-party panel already had areadymethod or attribute. I did check all the third-party panels listed in the Debug Toolbar documentation, and none of them would be affected by this change. However, if there are still backwards compatibility concerns about the addition of this method, I can rework this part of the PR.