GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-130052: Fix some exceptions on error paths in _testexternalinspection by sergey-miryanov · Pull Request #130053 · python/cpython Conversation
|
|
cpython/Modules/_testexternalinspection.c Line 917 in 67072cc
Refcnt doesn't used - maybe we may remove this reading? cpython/Modules/_testexternalinspection.c Line 1529 in 67072cc
This is a typo - should I fix them? get_stack_trace should be get_async_stack_trace
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@pablogsal Please take a look. I saw your fixed some calls (#129557), I added a few more exceptions. I also added a check of the result before it is used. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@sergey-miryanov: Why did you mark this PR as a draft? Do you expect to add more changes? Or is there something else? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@vstinner oops! I forgot to un-draft it. Thanks! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (proc_ref == 0) { | ||
| PyErr_SetString(PyExc_PermissionError, "Cannot get task for PID"); | ||
| if (!PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_PermissionError, "Cannot get task for PID"); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Would it be posssible to move this code inside pid_to_task()? I'm not convinced that PermissionError is appropriate, I would suggest RuntimeError instead.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I believe it is redundant here. I will remove this check. But I saw on the internets that task_for_pid may return code == 5 (os/kern failure) that we don't check - should we?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh! I think PyExc_PermissionError is just exactly for this. I will move it to pid_to_task.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I kept PyExc_PermissionError and removed redundant check. Should I replace PermissionError with RuntimeError?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <[email protected]>
|
@vstinner Please take a look. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @sergey-miryanov for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, @sergey-miryanov and @vstinner, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@vstinner It looks like a lot of changes were made on 3.14 branch and not on 3.13. Should we backport this PR then? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The automated backport failed because of merge conflicts. @sergey-miryanov: Can you please try to backport this change manually to 3.13? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@vstinner I tried yesterday yet. There are commits that not backported: Should I backport them all? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@pablogsal: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
3.13 has half the test cases because there is no async-related introspection and most of the bugs we have been fixing are in the new code. I will try to go over the PRs and see if anything applies to the other code that we had |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Reminder about backporting. @sergey-miryanov |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm afraid it is on @pablogsal side now. I'm not against of backporting those commits and if we decide to do this I'm ready to work on backporting. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
If it was decided to not backport this, please remove the "needs backport to 3.13" label. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It is on @pablogsal side to take decision of backporting this. I will remove "need backport@ label for now. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.