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

Does calling the macro from two different functions create two different objects? #3

Closed
daniil-berg opened this issue Nov 19, 2023 · 4 comments

Comments

@daniil-berg
Copy link

I was experimenting with this crate and came across behavior that I did not expect, but I am not sure, if this is a bug or if my expectation was wrong.

The way I understood it from the documentation, the first time I call the macro get_or_init with a specific type, a singleton is created. This seems to work fine, when dealing with a single function, like in the example. But consider the following simple code example, where I attempt to have a generic registry singleton (just a hash map):

use std::collections::HashMap;
use std::fmt::Display;
use std::hash::Hash;
use std::sync::RwLock;

use generic_singleton::get_or_init;


fn add_to_registry<T>(key: T, value: T) -> bool
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    let registry = get_or_init!(|| RwLock::new(HashMap::<T, T>::new()));
    if registry.read().unwrap().contains_key(&key) {
        false
    } else {
        registry.write().unwrap().insert(key, value);
        true
    }
}

fn get_from_registry<T>(key: T) -> T
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    let registry = get_or_init!(|| RwLock::new(HashMap::<T, T>::new()));
    if !registry.read().unwrap().contains_key(&key) {
        panic!("No such key registered: {}", key)
    }
    *registry.read().unwrap().get(&key).unwrap()
}

fn main() {
    assert!(add_to_registry::<u32>(1, 10));
    assert!(!add_to_registry::<u32>(1, 20));
    let val = get_from_registry::<u32>(1);
    assert_eq!(val, 10);
}

The get_from_registry call causes a panic with No such key registered: 1.

I expected it to return the value 10 since the type involved is the exact same: The underlying data structure is a HashMap<u32, u32>, so I thought the macro would get me the same singleton that I successfully created in the first add_to_registry call. Apparently that is not the case.

Am I misunderstanding something? If so, how would I have to approach this, if I wanted such a generic "registry"-like singleton?

@WalterSmuts
Copy link
Owner

WalterSmuts commented Nov 19, 2023

Does calling the macro from two different functions create two different objects? #3

Yes.

Am I misunderstanding something?

Yes, although I don't blame you. The docs could be more clear. Each occurrence of the get_or_init! macro causes a new static map to be initialised. I.e. a separate use of get_or_init! macro would refer to a separate singleton.

If so, how would I have to approach this, if I wanted such a generic "registry"-like singleton?

You need to factor the access into a single function. Something like:

use generic_singleton::get_or_init;
use std::collections::HashMap;
use std::fmt::Display;
use std::hash::Hash;
use std::sync::RwLock;

fn use_my_generic_singleton<T, U>(mut f: impl FnMut(&RwLock<HashMap<T, T>>) -> U) -> U
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    let my_generic_singleton = get_or_init!(|| RwLock::new(HashMap::<T, T>::new()));
    f(my_generic_singleton)
}

fn add_to_registry<T>(key: T, value: T) -> bool
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    use_my_generic_singleton(|registry| {
        if registry.read().unwrap().contains_key(&key) {
            false
        } else {
            registry.write().unwrap().insert(key, value);
            true
        }
    })
}

fn get_from_registry<T>(key: T) -> T
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    use_my_generic_singleton(|registry| {
        if !registry.read().unwrap().contains_key(&key) {
            panic!("No such key registered: {}", key)
        }
        *registry.read().unwrap().get(&key).unwrap()
    })
}

fn main() {
    assert!(add_to_registry::<u32>(1, 10));
    assert!(!add_to_registry::<u32>(1, 20));
    let val = get_from_registry::<u32>(1);
    assert_eq!(val, 10);
}

Although the above seems awfully complicated and I've not even convinced myself it makes sense. Will think about it some more but there's at least something for you for now.

I think I should probably update the docs or add a convenience method or perhaps both...

@daniil-berg
Copy link
Author

daniil-berg commented Nov 19, 2023

@WalterSmuts Thanks a lot for the clarification and your example workaround.

Your suggestion of using a closure would probably be the most flexible in this scenario. I suppose, if all I want is the add/get functionality, I could also go the opposite route and put that logic in a single not-so-nice non-public function and have two friendly public interfaces for it like so: (Thinking about this in the context of library code.)

use std::collections::HashMap;
use std::fmt::Display;
use std::hash::Hash;
use std::sync::RwLock;

use generic_singleton::get_or_init;


trait MyItem: Eq + Copy + Display + Hash + Send + Sync + 'static {}

impl<T> MyItem for T
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static
{}


fn use_registry<T: MyItem>(key: T, value: Option<T>) -> T {
    let registry = get_or_init!(|| RwLock::new(HashMap::<T, T>::new()));
    match value {
        None => {
            if !registry.read().unwrap().contains_key(&key) {
                panic!("No such key registered: {}", key)
            }
            *registry.read().unwrap().get(&key).unwrap()
        }
        Some(value) => {
            if registry.read().unwrap().contains_key(&key) {
                panic!("Key already registered: {}", key)
            }
            registry.write().unwrap().insert(key, value);
            value
        }
    }
}

pub fn add_to_registry<T: MyItem>(key: T, value: T) -> T {
    use_registry(key, Some(value))
}

pub fn get_from_registry<T: MyItem>(key: T) -> T {
    use_registry(key, None)
}

fn main() {
    assert_eq!(add_to_registry::<u32>(1, 10), 10);
    assert_eq!(get_from_registry::<u32>(1), 10);
}

This also runs as expected.


I still wonder, if it is possible to somehow achieve something like what I expected in the first place. I am very inexperienced with Rust still. And I have not looked deeper into your code yet, so I don't know if this could be added in as an additional feature or something, i.e. being able to "share" that singleton across functions.

But maybe you can dismiss this idea right off the bat, if you know it isn't possible. Maybe I'll gain some more insight, after going through your code.

Either way, this crate is cool and useful to me as is.

@WalterSmuts
Copy link
Owner

I still wonder, if it is possible to somehow achieve something like what I expected in the first place.

It is but it'll probably lead to unwanted outcomes... It was like that in version 1.3 but had to add the following documentation:

DO NOT USE WITH A TYPE YOUR CRATE DOES NOT PRIVATELY OWN!!! The map is shared across crates, so if you use a type that is publicly available, then another crate will be able to mutate your singleton, breaking whichever rules you’ve got in place and vice-versa. Use the new type pattern if you need to use a public struct in the map.

It's a bit of a foot-gun for users where the workaround is just creating your own wrapping function (This is probably what you wanted BTW; ignore the other example):

use std::collections::HashMap;
use std::fmt::Display;
use std::hash::Hash;
use std::sync::RwLock;

use generic_singleton::get_or_init;

fn get_my_generic_singleton<T>() -> &'static RwLock<HashMap<T, T>>
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    get_or_init!(|| RwLock::new(HashMap::<T, T>::new()))
}

fn add_to_registry<T>(key: T, value: T) -> bool
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    let registry = get_my_generic_singleton();
    if registry.read().unwrap().contains_key(&key) {
        false
    } else {
        registry.write().unwrap().insert(key, value);
        true
    }
}

fn get_from_registry<T>(key: T) -> T
where
    T: Eq + Copy + Display + Hash + Send + Sync + 'static,
{
    let registry = get_my_generic_singleton();
    if !registry.read().unwrap().contains_key(&key) {
        panic!("No such key registered: {}", key)
    }
    *registry.read().unwrap().get(&key).unwrap()
}

fn main() {
    assert!(add_to_registry::<u32>(1, 10));
    assert!(!add_to_registry::<u32>(1, 20));
    let val = get_from_registry::<u32>(1);
    assert_eq!(val, 10);
}

@daniil-berg
Copy link
Author

daniil-berg commented Nov 19, 2023

@WalterSmuts Ah, of course, just grabbing the singleton from a separate function makes much more sense. Thank you.

Interesting side note about the 🦶 🔫 with public types. Good to know that this is no longer an issue.

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

No branches or pull requests

2 participants