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

add pyo3::sync::with_critical_section_ptr #4922

Closed
wants to merge 2 commits into from

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Feb 18, 2025

This adds a public wrapper for the critical section C API that accepts a raw FFI pointer and refactors with_critical_section to use this new function.

While working on #4921 I thought I needed this but later realized I don't. Still, I think it's worth providing both the safe wrapper and unsafe wrapper in the public API, since sometimes you only have a raw pointer and it would be annoying to have to reimplement this every time.

@davidhewitt
Copy link
Member

Unless there is a very clear need for an API taking a raw pointer, I would prefer not to have this. In particular, unsafe functions taking closures are problematic because the unsafe easily infects the closure accidentally:

unsafe {
    with_critical_section_ptr(p, || {
        /* unsuspecting user uses unsafe APIs here due to top level unsafe block */
    })
}

The usual resolution is to require a parameter to be unsafely constructed instead, which happens to also fit our existing API:

with_critical_section(
    unsafe { &Borrowed::from_ptr(p) },
    || { /* no unsafe allowed here */ }
)

... so I think if anything, our existing API is preferred.

@ngoldbaum
Copy link
Contributor Author

Fair enough, you have a better feeling for this sort of thing than I do.

@ngoldbaum ngoldbaum closed this Feb 19, 2025
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.

2 participants