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.

Sign up for GitHub

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

Make PyDateTime_IMPORT FFI wrapper thread-safe #4623

Merged
merged 3 commits into from
Oct 20, 2024
Merged

Make PyDateTime_IMPORT FFI wrapper thread-safe #4623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ef8010
Make PyDateTime_IMPORT FFI wrapper thread-safe
ngoldbaum Oct 15, 2024
011364e
add changelog entry
ngoldbaum Oct 15, 2024
8e02786
add error checking for PyCapsule_Import call
ngoldbaum Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/4623.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* The FFI wrapper for the PyDateTime_IMPORT macro is now thread-safe.
43 changes: 30 additions & 13 deletions pyo3-ffi/src/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{PyObject, PyObject_TypeCheck, PyTypeObject, Py_TYPE};
use std::os::raw::c_char;
use std::os::raw::c_int;
use std::ptr;
use std::sync::Once;
use std::{cell::UnsafeCell, ffi::CStr};
#[cfg(not(any(PyPy, GraalPy)))]
use {crate::Py_hash_t, std::os::raw::c_uchar};
Expand Down Expand Up @@ -602,21 +603,32 @@ pub const PyDateTime_CAPSULE_NAME: &CStr = c_str!("datetime.datetime_CAPI");
/// `PyDateTime_IMPORT` is called
#[inline]
pub unsafe fn PyDateTimeAPI() -> *mut PyDateTime_CAPI {
*PyDateTimeAPI_impl.0.get()
*PyDateTimeAPI_impl.ptr.get()
}

/// Populates the `PyDateTimeAPI` object
pub unsafe fn PyDateTime_IMPORT() {
// PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
// `PyCapsule_Import` will behave unexpectedly in pypy.
#[cfg(PyPy)]
let py_datetime_c_api = PyDateTime_Import();

#[cfg(not(PyPy))]
let py_datetime_c_api =
PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *mut PyDateTime_CAPI;
if !PyDateTimeAPI_impl.once.is_completed() {
// PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
// `PyCapsule_Import` will behave unexpectedly in pypy.
#[cfg(PyPy)]
let py_datetime_c_api = PyDateTime_Import();

#[cfg(not(PyPy))]
let py_datetime_c_api =
PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *mut PyDateTime_CAPI;

if py_datetime_c_api.is_null() {
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can PyDateTime_Import ever fail? I guess if the return value is null then it might be unhelpful to continue to set the once. 🤔

Copy link
Contributor Author

@ngoldbaum ngoldbaum Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the docs for PyCapsule_Import imply that it might set an exeption and fail, but then again the definition of the PyDateTime_IMPORT macro doesn't do any error checking:
https://github.com/python/cpython/blob/12eaadc0ad33411bb02945d700b6ed7e758bb188/Include/datetime.h#L199-L200

so I think in practice it can't fail. Also, our PyDateTime_IMPORT FFI wrapper doesn't return anything (following CPython...), so we would need to change the API or tell people they need to call PyErr_Occurred after every import call, which would be kind of awful, especially for an issue that CPython itself doesn't seem to worry about.

I commented on an upstream issue about this: python/cpython#122184 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented there, I think it can fail. I think it's good enough here to add

Suggested change
if py_datetime_c_api.is_null() {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, easy enough to add the error check

*PyDateTimeAPI_impl.0.get() = py_datetime_c_api;
// Protect against race conditions when the datetime API is concurrently
// initialized in multiple threads. UnsafeCell.get() cannot panic so this
// won't panic either.
PyDateTimeAPI_impl.once.call_once(|| {
*PyDateTimeAPI_impl.ptr.get() = py_datetime_c_api;
});
}
}

#[inline]
Expand Down Expand Up @@ -735,8 +747,13 @@ extern "C" {

// Rust specific implementation details

struct PyDateTimeAPISingleton(UnsafeCell<*mut PyDateTime_CAPI>);
struct PyDateTimeAPISingleton {
once: Once,
ptr: UnsafeCell<*mut PyDateTime_CAPI>,
}
unsafe impl Sync for PyDateTimeAPISingleton {}

static PyDateTimeAPI_impl: PyDateTimeAPISingleton =
PyDateTimeAPISingleton(UnsafeCell::new(ptr::null_mut()));
static PyDateTimeAPI_impl: PyDateTimeAPISingleton = PyDateTimeAPISingleton {
once: Once::new(),
ptr: UnsafeCell::new(ptr::null_mut()),
};
Loading