◐ Shell
reader mode source ↗
Skip to content

gh-130052: Fix some exceptions on error paths in _testexternalinspection#130053

Merged
vstinner merged 10 commits into
python:mainfrom
sergey-miryanov:gh-130052-test-external-inspection-setexception
Feb 20, 2025
Merged

gh-130052: Fix some exceptions on error paths in _testexternalinspection#130053
vstinner merged 10 commits into
python:mainfrom
sergey-miryanov:gh-130052-test-external-inspection-setexception

Conversation

@sergey-miryanov

@sergey-miryanov sergey-miryanov commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

Some error paths don't set Exceptions.
This PR fixes them.

@sergey-miryanov

sergey-miryanov commented Feb 13, 2025

Copy link
Copy Markdown
Contributor Author

read_ptr(pid, task_address + sizeof(Py_ssize_t), &refcnt);

Refcnt doesn't used - maybe we may remove this reading?

"get_stack_trace is not supported on this platform");

This is a typo - should I fix them? get_stack_trace should be get_async_stack_trace

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

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

@vstinner

Copy link
Copy Markdown
Member

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

@sergey-miryanov

sergey-miryanov commented Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

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

@sergey-miryanov sergey-miryanov marked this pull request as ready for review February 17, 2025 13:13
@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

@vstinner Please take a look.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label Feb 20, 2025
@vstinner vstinner enabled auto-merge (squash) February 20, 2025 17:05
@vstinner vstinner merged commit 69426fc into python:main Feb 20, 2025
@miss-islington-app

Copy link
Copy Markdown

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

@miss-islington-app

Copy link
Copy Markdown

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

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

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

@vstinner

Copy link
Copy Markdown
Member

The automated backport failed because of merge conflicts.

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

@sergey-miryanov

sergey-miryanov commented Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

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

@vstinner

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

@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

@serhiy-storchaka

Copy link
Copy Markdown
Member

Reminder about backporting. @sergey-miryanov

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

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.

@serhiy-storchaka

Copy link
Copy Markdown
Member

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

@sergey-miryanov

Copy link
Copy Markdown
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants