-
Notifications
You must be signed in to change notification settings - Fork 0
Enable Free Threading #9
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
Conversation
…ildwheel to include 3.14rc2, and handle new pandas warning, and mark unsupported packages as < 3.14.
Not yet available
…ulti-phase init and accessed via GetModuleState()
…thonf into ci314t_sccache
src/duckdb_py/module_state.cpp
Outdated
| } | ||
|
|
||
| shared_ptr<DuckDBPyConnection> DuckDBPyModuleState::GetDefaultConnection() { | ||
| lock_guard<mutex> guard(default_connection_mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the GIL held here? I don't know pybind11 well enough to tell. If it is then you have to explicitly release it before locking a mutex, which is a possibly blocking operation. Otherwise you might deadlock with the GIL (or the garbage collector on the free-threaded build).
This is what MutexExt in PyO3 does. I don't know if there's something equivalent in pybind11. There probably should be if there isn't!
If you don't hold the GIL at this point then disregard all that. But also double-check everywhere else you are doing a possibly-blocking call with a standard library synchronization primitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking this through...
For context: I'm starting from code that is "GIL" safe, so mostly trying to preserve existing behavior / not change anything.
The GIL should be held in these paths; I didn't add a check, but I'll add a todo to consider a safety check.
The mutexes were just placeholders: next commit replaces them with an ifdef check for free threading and then either a mutex or a scoped_critical_section.
No description provided.