Skip to content

Improve the error reporting for inc_ref GIL failures - #4427

Merged
henryiii merged 5 commits into
pybind:masterfrom
EthanSteinberg:better_error_on_gil_error
Dec 30, 2022
Merged

Improve the error reporting for inc_ref GIL failures#4427
henryiii merged 5 commits into
pybind:masterfrom
EthanSteinberg:better_error_on_gil_error

Conversation

@EthanSteinberg

@EthanSteinbergEthanSteinberg commented Dec 27, 2022•
edited by henryiii
Loading

Copy link
Copy Markdown
Collaborator

Description

In 2.10.2, we added a new feature to catch UB when inc_ref or dec_ref are called without the GIL. This functionality works great, but the error reporting via an exception is very suboptimal as those exceptions get called in weird environments, triggering weird crashes.

This PR adds some good old-fashioned fprintf statements to help users understand what is going on. I want to use fprintf here because that's generally the most reliable method in all sorts of environments (especially when iostream might not be initialized properly).

Closes#4426

Suggested changelog entry:

Improved error messages when inc_ref/dec_ref are called with an invalid GIL state.

@EthanSteinberg

EthanSteinberg commented Dec 27, 2022

Copy link
Copy Markdown
CollaboratorAuthor

@Skylion007@henryiii I know this sucks, but I think you might want to push this out ASAP given all the issues with 2.10.2

Might be worth rereleasing 2.10.2 with this patch or pushing out 2.10.3

Skylion007
Skylion007 reviewed Dec 27, 2022
View reviewed changes
Comment threadinclude/pybind11/pytypes.h Outdated
Skylion007
Skylion007 reviewed Dec 27, 2022
View reviewed changes
Comment threadinclude/pybind11/pytypes.h Outdated
rwgk
rwgk reviewed Dec 27, 2022
View reviewed changes

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry a lot of comments, but really only about details. The general approach is great! Thanks again for jumping in making this better.

Comment threadinclude/pybind11/pytypes.h Outdated
Comment threadinclude/pybind11/pytypes.h Outdated
Comment threadinclude/pybind11/pytypes.h Outdated
Comment threadinclude/pybind11/pytypes.h Outdated
@rwgk

rwgk commented Dec 28, 2022

Copy link
Copy Markdown
Collaborator

I don't remember seeing the pypy-3.7 • windows-2022 • x64 CI failure. Rerunning to be sure it's a flake.

@EthanSteinberg

EthanSteinberg commented Dec 28, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

I believe all of the comments are now resolved. Thanks for the feedback and reviews!

rwgk
rwgk approved these changes Dec 28, 2022
View reviewed changes
Comment threaddocs/advanced/misc.rst Outdated
@rwgkrwgk mentioned this pull request Dec 28, 2022
Merged
rwgk
rwgk reviewed Dec 29, 2022
View reviewed changes
Comment threaddocs/advanced/misc.rst Outdated
@rwgk

rwgk commented Dec 30, 2022

Copy link
Copy Markdown
Collaborator

Could someone else please merge this? (@henryiii or @Skylion007)
I only have a very unstable internet connection for the next few days. In case there is something unexpected, I may not be able to respond quickly.

@henryiii
henryiii merged commit 60f02f5 into pybind:masterDec 30, 2022
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Dec 30, 2022
@EthanSteinberg
EthanSteinberg deleted the better_error_on_gil_error branch December 30, 2022 18:47
rwgk added a commit to google/clif that referenced this pull request Jan 1, 2023
@rwgk
Use PyType_Check() instead of direct comparison with PyType_Type …
ef64abf
…pointer.
Context:
* pybind/pybind11#4427 (comment)
> I don't think your logic ... is actually correct? I don't think it would handle metaclasses right.
PiperOrigin-RevId: 498690703
@rwgkrwgk mentioned this pull request Jan 1, 2023
Merged
henryiii pushed a commit that referenced this pull request Jan 2, 2023
@EthanSteinberg@henryiii
fix: improve the error reporting for inc_ref GIL failures (#4427)
e414c4b
* First
* Fixs
* Improve
* Additional assertions comment
* Improve docs
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Jan 3, 2023
@rwgkrwgk mentioned this pull request Feb 11, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rwgkrwgkrwgk approved these changes
@Skylion007Skylion007Skylion007 left review comments
@henryiiihenryiiihenryiii approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST]: Improve error logging with new inc_ref() feature

4 participants

@EthanSteinberg@rwgk@Skylion007@henryiii