The Wayback Machine - https://web.archive.org/web/20211027233409/https://github.com/python/cpython/pull/27934
Skip to content
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

Jump to bottom

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

Merged
merged 3 commits into from Aug 31, 2021
Merged

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

merged 3 commits into from Aug 31, 2021

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 24, 2021 •
edited by miss-islington

  • 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
@erlend-aasland
Acquire GIL at the very start of callbacks; release at the very end
e4f4679 
Exception: in the trace callback, do a sanity check before attempting to
hold the GIL.
@erlend-aasland erlend-aasland requested a review from encukou Aug 24, 2021
@erlend-aasland erlend-aasland mentioned this pull request Aug 25, 2021
Merged
@erlend-aasland
Copy link
Contributor Author

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

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

@encukou
Copy link
Member

@encukou encukou commented Aug 31, 2021 •
edited

_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-aasland
Add explanatory comment about mysterious PyErr_Occurred()
2a2152d
@erlend-aasland
Address review
d42eb38 
- 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

@erlend-aasland erlend-aasland 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.

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

PTAL :)

encukou
encukou approved these changes Aug 31, 2021
View changes
Copy link
Member

@encukou encukou left a comment

Looks good, thank you!

@erlend-aasland
Copy link
Contributor Author

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

Thanks, and thank you for reviewing!

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 31, 2021

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 31, 2021

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

@miss-islington miss-islington merged commit 001ef46 into python:main Aug 31, 2021
11 checks passed
@erlend-aasland erlend-aasland deleted the sqlite-cleanup-gil branch Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou

@berkerpeksag berkerpeksag

Assignees
No one assigned
Labels
CLA signed 🤖 automerge skip news
Projects
None yet
Milestone
No milestone
5 participants
@erlend-aasland @encukou @miss-islington @the-knights-who-say-ni @bedevere-bot