Make PyDateTime_IMPORT FFI wrapper thread-safe #4623
Conversation
|
|
||
| PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *mut PyDateTime_CAPI | ||
| }; | ||
|
|
There was a problem hiding this comment.
Can PyDateTime_Import ever fail? I guess if the return value is null then it might be unhelpful to continue to set the once. 🤔
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I commented there, I think it can fail. I think it's good enough here to add
| if py_datetime_c_api.is_null() { | |
| return; | |
| } |
There was a problem hiding this comment.
ok, easy enough to add the error check
ngoldbaum
force-pushed
the
datetime-import-thread-safety
branch
from
afb9c99 to
8e02786
Compare
Use a
std::sync::onceto ensure theUnsafeCellis only ever written to once. I don't think it's possible for setting the pointer to panic, so I don't think it makes a difference to usecall_oncevscall_once_force.This is the only singleton like this in the FFI wrappers I can find by grepping.