Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gpshead commentedMay 8, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I left the empty |
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.
My remark about PyThread_release_lock() is unrelated to your PR.
| if (!(aLock&&LeaveNonRecursiveMutex((PNRMUTEX)aLock))) | ||
| dprintf(("%lu: Could not PyThread_release_lock(%p) error: %ld\n",PyThread_get_thread_ident(),aLock,GetLastError())); | ||
| if (aLock) { |
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.
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.
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'd be surprised to ever see this called withaLock == 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.
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.
Sure. I created#92586
No description provided.