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 is_safe to rust UUID #70

Merged
merged 18 commits into from
Nov 21, 2024
Merged

add is_safe to rust UUID #70

merged 18 commits into from
Nov 21, 2024

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Sep 15, 2024

I know little about rust, hope this is not a very bad idea.

close #69

@trim21 trim21 changed the title 挨打 add is_safe to rust UUID Sep 15, 2024
@trim21 trim21 mentioned this pull request Sep 15, 2024
src/lib.rs Outdated Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented Sep 15, 2024

also, are all of our generators multiprocess safe?

@aminalaee
Copy link
Owner

I'm not sure if the whole approach is correct.
Looking at the Python and C source code, the is_safe is determined that when generating a UUID. I don't think just passing the UUID to the Python Enum will determine if it is safe or not.

@trim21
Copy link
Contributor Author

trim21 commented Sep 19, 2024

I'm not sure if the whole approach is correct. Looking at the Python and C source code, the is_safe is determined that when generating a UUID. I don't think just passing the UUID to the Python Enum will determine if it is safe or not.

what's why I ask #70 (comment) for many times, are our generators multiprocess safe?

uuid v3 and uuid v5 are hash based so thy are always safe. uuid v1 and v6 are time based, and uuid v7 are time and random based, uuid v4 are random based, are they safe of not?

If they are all safe, we can just return SafeUUID.safe. otherwise we need to pass then in uuid generator function by condition, for example, different arch and different OS.

If you don't know, then we can just return SafeUUID.unknown.

@aminalaee
Copy link
Owner

I don't think it's that straightforward. Even our uuid4 can be unsafe:

import sys
import uuid_utils
sys.modules["uuid"] = uuid_utils
import os

import uuid

a = uuid.uuid4()
print(f"a = {a}")
os.fork()
b = uuid.uuid4()
print(f"b = {b}")

But what I think we can do is:

  • Define SafeUUID Enum in Rust
  • For now just return Unknown until a better approach is found

@trim21
Copy link
Contributor Author

trim21 commented Sep 19, 2024

Even our uuid4 can be unsafe:

Yes, what's what I'm asking, which means it's unsafe or unknown.

Define SafeUUID Enum in Rust

Since it's already defined in stdlib I don't think we should define it in rust again, some user may compare it with stdlib SafeUUID

@trim21
Copy link
Contributor Author

trim21 commented Sep 19, 2024

I find that stdlib uuid return unknown for even v3

@trim21
Copy link
Contributor Author

trim21 commented Oct 9, 2024

/ping

@trim21
Copy link
Contributor Author

trim21 commented Oct 26, 2024

.into_ptr() will wrap ptr with ManuallyDrop to avoid dec_ref on drop, keep refcnt correct.

@aminalaee
Copy link
Owner

I will do some benchmark tests locally and get back to this PR. Sorry it took so long.

@trim21
Copy link
Contributor Author

trim21 commented Nov 4, 2024

it's a benchmark for AtomicPtr.load...

@aminalaee
Copy link
Owner

not really, it's a benchmark how PyO3 handles this and how it will affect our benchmarks.

@trim21 trim21 mentioned this pull request Nov 4, 2024
@trim21
Copy link
Contributor Author

trim21 commented Nov 5, 2024

it will add a new PyMethodDefType::Getter to method defs, and pyo3::callback::convert or BoundRef::ref_from_ptr are noop for *mut ffi::PyObject

9c9
<     prelude::*, pyclass::CompareOp, types::{PyBytes, PyDict},
---
>     ffi, prelude::*, pyclass::CompareOp, types::{PyBytes, PyDict},
12,13d11
< use std::hash::Hasher;
< use std::sync::atomic::{AtomicU64, Ordering};
14a13,14
> use std::{hash::Hasher, sync::atomic::AtomicPtr};
> use std::{ptr::null_mut, sync::atomic::{AtomicU64, Ordering}};
412a413,415
>     fn is_safe(&self) -> *mut ffi::PyObject {
>         return SAFE_UUID_UNKNOWN.load(Ordering::Relaxed);
>     }
816a820,828
>                 ::pyo3::impl_::pyclass::MaybeRuntimePyMethodDef::Static(
>                     ::pyo3::class::PyMethodDefType::Getter(
>                         ::pyo3::class::PyGetterDef::new(
>                             c"is_safe",
>                             UUID::__pymethod_get_is_safe__,
>                             c"",
>                         ),
>                     ),
>                 ),
1923a1936,1954
>     unsafe fn __pymethod_get_is_safe__(
>         py: ::pyo3::Python<'_>,
>         _slf: *mut ::pyo3::ffi::PyObject,
>     ) -> ::pyo3::PyResult<*mut ::pyo3::ffi::PyObject> {
>         #[allow(clippy::let_unit_value)]
>         let mut holder_0 = ::pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
>         let result = ::pyo3::callback::convert(
>             py,
>             UUID::is_safe(
>                 ::pyo3::impl_::extract_argument::extract_pyclass_ref::<
>                     UUID,
>                 >(
>                     ::pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf).0,
>                     &mut holder_0,
>                 )?,
>             ),
>         );
>         result
>     }
2603a2635
> static SAFE_UUID_UNKNOWN: AtomicPtr<ffi::PyObject> = AtomicPtr::new(null_mut());
2650a2683,2694
>     let safe_uuid_unknown = Python::with_gil(|py| {
>         return PyModule::import_bound(py, "uuid")
>             .unwrap()
>             .getattr("SafeUUID")
>             .unwrap()
>             .unbind()
>             .bind(py)
>             .getattr("unknown")
>             .unwrap()
>             .unbind();
>     });
>     SAFE_UUID_UNKNOWN.store(safe_uuid_unknown.into_ptr(), Ordering::Relaxed);

@trim21
Copy link
Contributor Author

trim21 commented Nov 10, 2024

👀

Copy link
Owner

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍

@aminalaee aminalaee merged commit f72cb24 into aminalaee:main Nov 21, 2024
21 checks passed
@aminalaee aminalaee mentioned this pull request Nov 21, 2024
@trim21 trim21 deleted the is-safw branch November 23, 2024 02:00
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.

typing doesn't match impl
2 participants