Skip to content

bpo-44991: Make GIL handling more explicit in sqlite3 callbacks #27934

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 31, 2021

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