Skip to content

bpo-42064: Pass module state to sqlite3 UDF callbacks #27456

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

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 29, 2021

  • Establish common callback context struct
  • Convert UDF callbacks to fetch module state from callback context

https://bugs.python.org/issue42064

Automerge-Triggered-By: GH:encukou

@erlend-aasland
Copy link
Contributor Author

A couple of questions, @encukou:

  • Do I need to ensure GIL is held in create_callback_context(), or can I get away with it like it is?
  • Ditto for free_callback_context()

@encukou
Copy link
Member

encukou commented Jul 30, 2021

Do I need to ensure GIL is held in create_callback_context(), or can I get away with it like it is?

There are very few funcitons in the C-API that you can call without the GIL held; usually they're the ones needed before a GIL is created. For PyMem_Malloc/PyMem_Free specifically, there's a a big red warning to answer your question ;)
PyMem_Malloc is configurable; the default allocator relies on the GIL for synchronization.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 30, 2021

There are very few funcitons in the C-API that you can call without the GIL held; usually they're the ones needed before a GIL is created. For PyMem_Malloc/PyMem_Free specifically, there's a a big red warning to answer your question ;)

Yes, I've seen it ;) But then why does this PR work as it is? (That is, without the GIL)

(FYI: I'll update the PR with GIL wrappers soon)

@encukou
Copy link
Member

encukou commented Jul 30, 2021

But then why does this PR work as it is?

I assume it's because you're not using multiple threads.
Even if you were, it might be rare to cause the memory allocator to run in two threads at once. But when it would, it'd crash quite spectacularly. (Or worse, if a malicious actor could cause it to happen.)

@erlend-aasland
Copy link
Contributor Author

But then why does this PR work as it is?

I assume it's because you're not using multiple threads.
Even if you were, it might be rare to cause the memory allocator to run in two threads at once. But when it would, it'd crash quite spectacularly. (Or worse, if a malicious actor could cause it to happen.)

Ah, right. Thanks!

I've updated the PR. PTAL :)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 6, 2021

On hold until #27613 has landed.

@encukou, this is now ready for review (again). PTAL.

@erlend-aasland erlend-aasland marked this pull request as draft August 6, 2021 07:41
@erlend-aasland erlend-aasland marked this pull request as ready for review August 6, 2021 19:16
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.

I found some more questionable uses of the GIL in existing code (see the issue). But let's not block this PR; it is an improvement.

@miss-islington miss-islington merged commit 9ed5231 into python:main Aug 24, 2021
@erlend-aasland erlend-aasland deleted the sqlite-callback-state/part1 branch August 24, 2021 13:03
@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing!

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