◐ Shell clean mode source ↗

gh-130052: Fix some exceptions on error paths in _testexternalinspection by sergey-miryanov · Pull Request #130053 · python/cpython

bedevere-app

@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.

@sergey-miryanov: Why did you mark this PR as a draft? Do you expect to add more changes? Or is there something else?

@vstinner oops! I forgot to un-draft it. Thanks!

		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");

Choose a reason for 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for 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.

Choose a reason for 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?

Co-authored-by: Victor Stinner <vstinner@python.org>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks @sergey-miryanov for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

Sorry, @sergey-miryanov and @vstinner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 69426fcee7fcecbe34be66d2c5b58b6d0ffe2809 3.13

@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?

The automated backport failed because of merge conflicts.

@sergey-miryanov: Can you please try to backport this change manually to 3.13?

@vstinner I tried yesterday yet. There are commits that not backported:

* 69426fcee7f gh-130052: Fix some exceptions on error paths in _testexternalinspection (#130053)
* 49b11033bd8 GH-91048: Correct error path in testexternalinspection (#129557)
* 633853004c5 gh-130050: Fix memory leaks in _testexternalinspection (#130051)
* 7eaef74561c gh-129430: Make walking vm regions more efficient in MacOS (#129494)
* be98fda7c66 gh-129223: Raise KeyError in search_map_for_section() if not found (#129262)
* 3a3a6b86f40 gh-129223: Do not allow the compiler to optimise away symbols for debug sections (#129225)
* 67d804b494d GH-91048: Don't attempt to run on FreeBSD (#129189)
* 188598851d5 GH-91048: Add utils for capturing async call stack for asyncio programs and enable profiling (#124640)
* f5b6356a11b GH-128563: Add new frame owner type for interpreter entry frames (GH-129078)
* 6d93690954d gh-125604: Move _Py_AuditHookEntry, etc. Out of pycore_runtime.h (gh-125605)
* b2afe2aae48 gh-123923: Defer refcounting for `f_executable` in `_PyInterpreterFrame` (#123924)

Should I backport them all?

@pablogsal: Modules/_testexternalinspection.c lacks many backports in the 3.13 branch. Is it a deliberate choice? Or should we backport these changes to 3.13?

@pablogsal: Modules/_testexternalinspection.c lacks many backports in the 3.13 branch. Is it a deliberate choice? Or should we backport these changes to 3.13?

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

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.

If it was decided to not backport this, please remove the "needs backport to 3.13" label.

It is on @pablogsal side to take decision of backporting this. I will remove "need backport@ label for now.