Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 24, 2021

  • acquire the GIL at the very start[1]
  • release the GIL at the very end

[1] The trace callback performs a sanity check before acquiring the GIL

https://bugs.python.org/issue44991

Automerge-Triggered-By: GH:encukou

Exception: in the trace callback, do a sanity check before attempting to
hold the GIL.
@erlend-aasland
Copy link
Contributor Author

FYI, I'll normalise the callback names in a separate PR.

@encukou
Copy link
Member

encukou commented Aug 31, 2021

_destructor is also a callback function. It should get & release the GIL itself. (The NULL check can be moved here as well, as other calls of free_callback_context are definitely with a non-NULL context.)
PyGILState_Ensure/PyGILState_Release can be then removed from both create_callback_context and free_callback_context. Those will always be called with the GIL held.

Erlend E. Aasland added 2 commits August 31, 2021 13:45
- since _destructor is a callback, wrap it in GIL lock/unlock
- remove GIL lock/unlock from create_callback_context()
@erlend-aasland
Copy link
Contributor Author

_destructor is also a callback function. It should get & release the GIL itself. (The NULL check can be moved here as well, as other calls of free_callback_context are definitely with a non-NULL context.)
PyGILState_Ensure/PyGILState_Release can be then removed from both create_callback_context and free_callback_context. Those will always be called with the GIL held.

Yes, that's cleaner. Thanks! See d42eb38.

PTAL :)

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@erlend-aasland
Copy link
Contributor Author

Thanks, and thank you for reviewing!

@miss-islington
Copy link
Contributor

@erlend-aasland: Status check is done, and it's a failure ❌ .

@miss-islington
Copy link
Contributor

@erlend-aasland: Status check is done, and it's a success ✅ .

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants