Skip to content

Remove redundant GIL acquisition during PyObject release#443

Open
atifaziz wants to merge 1 commit intotonybaloney:mainfrom
atifaziz:x-redundant-gil
Open

Remove redundant GIL acquisition during PyObject release#443
atifaziz wants to merge 1 commit intotonybaloney:mainfrom
atifaziz:x-redundant-gil

Conversation

@atifaziz
Copy link
Copy Markdown
Collaborator

@atifaziz atifaziz commented May 3, 2025

This PR removes a redundant GIL acquisition during the release of a PyObject.

@tonybaloney
Copy link
Copy Markdown
Owner

The GIL is required and I want to avoid a possible race condition from the check above.

The scenario is a custom __del__ method on a heap allocated class which would be called when the object is disposed and the GIL assertion would cause a hard crash. It's possible we're just not testing that right now (or aren't testing the race)

@atifaziz
Copy link
Copy Markdown
Collaborator Author

atifaziz commented May 5, 2025

The GIL is required and I want to avoid a possible race condition from the check above.

The scenario is a custom __del__ method on a heap allocated class which would be called when the object is disposed and the GIL assertion would cause a hard crash. It's possible we're just not testing that right now (or aren't testing the race)

If it's this tricky, then I think we should add a comment, but I am still having a tough time seeing the race condition. The GIL class is designed with TLS in mind for avoiding nested acquisitions for the same thread. If GIL.IsAcquired returned ture, it means that the calling thread already has the GIL. If there's a thread-switch between the GIL.IsAcquired check and GIL.Acquire then it wouldn't matter because other threads would see their own state of GIL acquisition. When the interrupted thread resumes, the result of its GIL.IsAcquired won't have changed. Am I missing something?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants