GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-130052: Fix some exceptions on error paths in _testexternalinspection by sergey-miryanov · Pull Request #130053 · python/cpython

Conversation

@sergey-miryanov

@sergey-miryanov sergey-miryanov commented Feb 12, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • Issue: Some error paths do not set Exceptions in _testexternalinspection #130052

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sergey-miryanov
Set exception in read_memory if not supported on the platform
23fdc5f
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting review labels Feb 12, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 12, 2025
Some error paths do not set Exceptions in _testexternalinspection #130052
Closed
@sergey-miryanov
sergey-miryanov marked this pull request as draft February 12, 2025 20:35
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 12, 2025
@sergey-miryanov sergey-miryanov mentioned this pull request Feb 12, 2025
Python 3.14.0a5: test_external_inspection fails on freethreading-debug build on s390x Linux #130035
Closed
@encukou encukou added the skip news label Feb 13, 2025
sergey-miryanov added 3 commits February 13, 2025 18:55
@sergey-miryanov
Merge branch 'main' into pythongh-130052-test-external-inspection-set…
9679c08
…exception
@sergey-miryanov
Add more exceptions to _testexternalinspection
5e15349
@sergey-miryanov
Fix PyErr_Format call
67072cc
@sergey-miryanov

sergey-miryanov commented Feb 13, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

cpython/Modules/_testexternalinspection.c

Line 917 in 67072cc

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

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

cpython/Modules/_testexternalinspection.c

Line 1529 in 67072cc

"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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov

sergey-miryanov commented Feb 13, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sergey-miryanov added 2 commits February 13, 2025 19:33
@sergey-miryanov
Fix small typo
b8c800c
@sergey-miryanov
Merge branch 'main' into pythongh-130052-test-external-inspection-set…
4830892
…exception
@vstinner

vstinner commented Feb 17, 2025

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov

sergey-miryanov commented Feb 17, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov
sergey-miryanov marked this pull request as ready for review February 17, 2025 13:13
@bedevere-app bedevere-app Bot added the awaiting review label Feb 17, 2025
@sergey-miryanov
Merge branch 'main' into pythongh-130052-test-external-inspection-set…
8bf3c22
…exception
vstinner
vstinner reviewed Feb 17, 2025
View reviewed changes
Comment thread Modules/_testexternalinspection.c Outdated
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");

@vstinner vstinner Feb 17, 2025

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sergey-miryanov sergey-miryanov Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

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

  • https://github.com/attilathedud/macos_task_for_pid
  • https://forums.developer.apple.com/forums/thread/701647

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sergey-miryanov sergey-miryanov Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

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.

Oh! I think PyExc_PermissionError is just exactly for this. I will move it to pid_to_task.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sergey-miryanov sergey-miryanov Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

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 kept PyExc_PermissionError and removed redundant check. Should I replace PermissionError with RuntimeError?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_testexternalinspection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testexternalinspection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

sergey-miryanov and others added 3 commits February 17, 2025 22:02
@sergey-miryanov @vstinner
Update Modules/_testexternalinspection.c
6dfc0f0
Co-authored-by: Victor Stinner <[email protected]>
@sergey-miryanov
Remove redundant check of pid_to_task result
e9ac75b
@sergey-miryanov
Fit lines to 80 columns
4fde8de
@sergey-miryanov

sergey-miryanov commented Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

@vstinner Please take a look.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner approved these changes Feb 20, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 20, 2025
@vstinner vstinner added the needs backport to 3.13 only 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

miss-islington-app Bot commented Feb 20, 2025

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.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 20, 2025
@miss-islington-app

miss-islington-app Bot commented Feb 20, 2025

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned vstinner Feb 20, 2025
@sergey-miryanov

sergey-miryanov commented Feb 20, 2025

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 21, 2025

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov

sergey-miryanov commented Feb 21, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 21, 2025

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Feb 21, 2025

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

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Aug 14, 2025

Copy link
Copy Markdown
Member

Reminder about backporting. @sergey-miryanov

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov

sergey-miryanov commented Aug 14, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Dec 26, 2025

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov

sergey-miryanov commented Feb 4, 2026

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sergey-miryanov sergey-miryanov removed the needs backport to 3.13 only security fixes label Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@vstinner vstinner

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@sergey-miryanov @vstinner @pablogsal @serhiy-storchaka @encukou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.