Skip to content

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Sep 25, 2024

This PR fixes #14105

As part of this fix I realized that my previous assumptions about gil_scoped_release + gil_scoped_acquire were wrong, they are fine to be nested. It's the gil_scoped_release that can't be nested in itself, that causes a crash

@Tishj
Copy link
Contributor Author

Tishj commented Sep 25, 2024

I think I'll do some digging through all the paths that lead to py::gil_scoped_release to verify that they are always called with the GIL held.

I would have never suspected nested py::gil_scoped_release structs to cause a crash, if it's not there it doesn't have to be released. pybind thinks differently it seems

…ell use it liberally to ensure the gil is held when a py::gil_scoped_release is encountered
@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 25, 2024 12:28
@Tishj Tishj marked this pull request as ready for review September 25, 2024 12:31
@Mytherin Mytherin merged commit d58cc56 into duckdb:main Sep 27, 2024
19 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

@Tishj Tishj mentioned this pull request Sep 30, 2024
2 tasks
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Oct 16, 2024
Less salt (duckdb/duckdb#14173)
[Python Dev] Make sure the GIL is released when the connection+db are being shut down (duckdb/duckdb#14113)
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Oct 16, 2024
Less salt (duckdb/duckdb#14173)
[Python Dev] Make sure the GIL is released when the connection+db are being shut down (duckdb/duckdb#14113)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
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.

Unit tests of built Python package fail on OSX (in conda-forge build)
2 participants