-
-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gracefully error out when there are concurrent profilers #692
Conversation
c8022e4
to
ac3f28d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #692 +/- ##
==========================================
+ Coverage 86.65% 86.67% +0.02%
==========================================
Files 52 52
Lines 2113 2117 +4
==========================================
+ Hits 1831 1835 +4
Misses 282 282 ☔ View full report in Codecov by Sentry. |
f0b8b96
to
e4e8653
Compare
b42e593
to
6537b6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello again @albertyw,
this patch permanently fixes the Django==5.1
& python^=3.12
pipeline issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to point out while this stop silk from interfering with processing a request, it also ends up not profiling that particular request. Prior to Python 3.11, the effect was similar but different (it would disable/stop the first profiler, and then start this current one). Neither outcome seems great, especially since this problem will happen when Django is processing multiple long-running requests at the same time (I've made a minimal repo here to highlight this issue) I think at a minimum, the documentation should tell users that this issue exists. In terms of a proper fix, I'm not sure at the moment if that's possible without using another profiler that properly supports multithreaded applications (such as https://github.com/sumerc/yappi) |
Thank you for pointing out this oversight, @robinchow. I'd take a look into yappi |
3.11 or 3.12? This code gives an error in python 3.12 but no error in 3.11: import cProfile
p1 = cProfile.Profile()
p1.enable()
p2 = cProfile.Profile()
p2.enable() |
@albertyw ah yes, apologies, prior to Python 3.12. |
Fixes #682
Unblocks #686