eclipse-uprotocol / up-rust

uProtocol Language Specific Library for Rust
Apache License 2.0
11 stars 9 forks source link

UUIDBuilder -- should it be a singleton as it is in the other language libs? (e.g. `up-java` and `up-cpp`) #58

Closed PLeVasseur closed 6 months ago

PLeVasseur commented 6 months ago

Howdy :wave:

@stevenhartley recently brought up on a PR going into up-client-zenoh-rust that the other language libs implement their UUID builder / factory as singletons to avoid an end-user accidentally foot-gunning themselves by using multiple UUIDBuilder within one uE.

He asked why the same wasn't done in up-rust. I had my thoughts as to why:

Making it a singleton in Rust forces upon the end user a certain way of interacting with the singleton and there are multiple (reasonable) ways of doing this in Rust. However, maybe it makes sense to give a "simple" path to an end-user not foot-gunning themselves with multiple UUIDBuilder, but allow the possibility for an advanced user to ensure it's a singleton themselves?

A couple of reasonable ways of doing this in Rust:

Looking to hear feedback on this, hopefully from @sophokles73, @AnotherDaniel, and @evshary

evshary commented 6 months ago

I'm fine with putting singleton in either up-rust or up-client-zenoh-rust. I'm wondering if there is any special use case we need to have multiple UUIDBuilder.

PLeVasseur commented 6 months ago

Bump =^)

Hoping to hear feedback from @sophokles73 and @AnotherDaniel in particular

sophokles73 commented 6 months ago

Sorry for the delay.

If I understand correctly, the main point here is that we want to be able to share a single instance of UUIDBuilder within a uEntity for creating UUIDs, right? If so, then I guess we should focus on making UUIDBuilder thread-safe first. After that we can discuss whether we want to use the Singleton pattern for sharing the instance or pass around a reference explicitly.

PLeVasseur commented 6 months ago

If I understand correctly, the main point here is that we want to be able to share a single instance of UUIDBuilder within a uEntity for creating UUIDs, right?

Yup, ideally to prevent the foot-gun of end-users who are developing uEs doing the "wrong" thing by e.g. creating a new UUIDBuilder and building a UUID from it on e.g. every publish.

If so, then I guess we should focus on making UUIDBuilder thread-safe first. After that we can discuss whether we want to use the Singleton pattern for sharing the instance or pass around a reference explicitly.

Makes sense.


I like the idea of allowing users of up-rust to create a new UUIDBuilder if they know what they're doing, but to maybe present a simpler or intention-revealing API for the 99% use-case of where, I, as a uE developer just want to be able to get the "correct" UUID for stamping onto my messages I'm sending.

Does that make sense?

stevenhartley commented 6 months ago

A UUID has to be a singleton, otherwise we will have 2 different rand_b generated for the same instance of a running uE (not good at all)

PLeVasseur commented 6 months ago

A UUID has to be a singleton, otherwise we will have 2 different rand_b generated for the same instance of a running uE (not good at all)

Agreed for the 99% case. IMHO it'd be nice to have an escape hatch for other cases, e.g. if I wanted to be able to use the UUIDBuilder inside of uStreamer for a purpose completely unrelated to stamping messages.

(I'm using it to uniquely stamp Rust channel senders so I can then hash them and keep them within a HashMap

Actually now that I think about it... that could also be a singleton in that case, since I need only use one.)

PLeVasseur commented 6 months ago

btw, to get actionable:

If so, then I guess we should focus on making UUIDBuilder thread-safe first.

Agreed on this point -- do you want to take a stab at this or should I?

stevenhartley commented 6 months ago

A UUID has to be a singleton, otherwise we will have 2 different rand_b generated for the same instance of a running uE (not good at all)

Agreed for the 99% case. IMHO it'd be nice to have an escape hatch for other cases, e.g. if I wanted to be able to use the UUIDBuilder inside of uStreamer for a purpose completely unrelated to stamping messages.

(I'm using it to uniquely stamp Rust channel senders so I can then hash them and keep them within a HashMap

Actually now that I think about it... that could also be a singleton in that case, since I need only use one.)

Well messages that go in and out of the streamer already have UUIDs in them, there shouldn't be a need to mess around with the IDs.

PLeVasseur commented 6 months ago

Yeah -- not messing with message IDs. Using UUIDBuilder to uniquely stamp the async channels to be able to store them and look them up efficiently.

I can walk you through the code sometime.

sophokles73 commented 6 months ago

A UUID has to be a singleton, otherwise we will have 2 different rand_b generated for the same instance of a running uE (not good at all)

just out of curiosity: why is that not good at all? Is there a real problem with a UE producing UUIDs with different random values? They will do that anyway if they are restarted ...

sophokles73 commented 6 months ago

btw, to get actionable:

If so, then I guess we should focus on making UUIDBuilder thread-safe first.

Agreed on this point -- do you want to take a stab at this or should I?

Be my guest :-) I had already spent some time on this but struggled a lot with the atomic semantics of Rust (or better: my lack of understanding them ;-)). We could always fall back to simply using a Mutex for guarding the counter but I would rather first try to get away with something less strict ...

stevenhartley commented 6 months ago

A UUID has to be a singleton, otherwise we will have 2 different rand_b generated for the same instance of a running uE (not good at all)

just out of curiosity: why is that not good at all? Is there a real problem with a UE producing UUIDs with different random values? They will do that anyway if they are restarted ...

We defined in the spec that the random number is used to differentiate an instance of the running uE, if there are 2 random numbers publishing at the same time (same source URI), the system could be confused if we want to correlate messages during a given runtime.

PLeVasseur commented 6 months ago

btw, to get actionable:

If so, then I guess we should focus on making UUIDBuilder thread-safe first.

Agreed on this point -- do you want to take a stab at this or should I?

Be my guest :-) I had already spent some time on this but struggled a lot with the atomic semantics of Rust (or better: my lack of understanding them ;-)). We could always fall back to simply using a Mutex for guarding the counter but I would rather first try to get away with something less strict ...

Hey @sophokles73 -- I took my shot :) Could you take a look at the approach taken in #74?

sophokles73 commented 6 months ago

@PLeVasseur can this be closed?