Skip to content

gh-88750: Remove the PYTHONTHREADDEBUG env var support. #92509

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 9, 2022
Merged

gh-88750: Remove the PYTHONTHREADDEBUG env var support. #92509

merged 2 commits into from
May 9, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented May 8, 2022

No description provided.

@gpshead gpshead added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 8, 2022
@gpshead gpshead requested a review from vstinner May 8, 2022 19:44
@gpshead
Copy link
Member Author

gpshead commented May 8, 2022 •
edited
Loading

I left the empty dprintf macro definition and its uses in the Python/thread_*.h files. I figure those can be cleaned up in a separate commit. Update: Added another commit removing them.

@gpshead gpshead linked an issue May 8, 2022 that may be closed by this pull request
Deprecate thread debugging PYTHONTHREADDEBUG=1 #88750
Closed
@gpshead gpshead self-assigned this May 8, 2022
vstinner
vstinner approved these changes May 9, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

My remark about PyThread_release_lock() is unrelated to your PR.

Python/thread_nt.h

if (!(aLock && LeaveNonRecursiveMutex((PNRMUTEX) aLock)))
dprintf(("%lu: Could not PyThread_release_lock(%p) error: %ld\n", PyThread_get_thread_ident(), aLock, GetLastError()));
if (aLock) {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, we have a special case to do nothing if the lock is not initialized? Only for the Windows implementation and only for PyThread_release_lock()? That's weird. I suggest to replace the if() with an assert(), but done in a separated PR. (I can do it if I don't forget.)

It's weird because the pthread implement has no special case for PyThread_release_lock() and so the code is not portable. Or maybe I misunderstood something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be surprised to ever see this called with aLock == NULL, I just figured I should preserve the exact semantics when removing the debug print. Agreed on the assert, but that does make more sense in a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I created #92586

@gpshead gpshead merged commit 6ed7c35 into python:main May 9, 2022
@gpshead gpshead deleted the goodbye-threaddebug branch May 9, 2022 23:03
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

Assignees

@gpshead gpshead

Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate thread debugging PYTHONTHREADDEBUG=1
3 participants
@gpshead @vstinner @bedevere-bot