-
Notifications
You must be signed in to change notification settings - Fork 398
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
Protect against deregistered profile functions in greenlet switches #700
Conversation
7cfb8b5
to
ef5e564
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.
Copilot reviewed 1 out of 3 changed files in this pull request and generated no suggestions.
Files not reviewed (2)
- news/700.bugfix.rst: Language not supported
- src/memray/_memray/tracking_api.cpp: Language not supported
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.
Just a few minor nits
When greenlet tracking is enabled it's possible that we run into a situation where the function that recreates the Python stack in our TLS variable after a greenlet switch is called **after** the profile function has been deactivated. In this case, recreating the Python stack is wrong as we are no longer tracking POP/PUSH events so when the stack is inspected later nothing guarantees that the frames are still valid. Signed-off-by: Pablo Galindo <[email protected]>
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.
LGTM. I've rebased this and made all of the changes I suggested in my comments above. Check my work and feel free to land if you don't have any objections.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
- Coverage 92.93% 92.92% -0.02%
==========================================
Files 95 95
Lines 11655 11675 +20
Branches 406 406
==========================================
+ Hits 10832 10849 +17
- Misses 823 826 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
When greenlet tracking is enabled is possible that we run into a
situation where the function that recreates the Python stack in our TLS
variable after a greenlet switch is called after the profile
function has been deactivated. In this case, recreating the Python stack
is wrong as we are no longer tracking POP/PUSH events so when the stack
is inspected later nothing guarantees that the frames are still valid.
Issue number of the reported bug or feature request: #
Describe your changes
A clear and concise description of the changes you have made.
Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.
Additional context
Add any other context about your contribution here.