search

eclipse-uprotocol / up-rust

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

register/unregiter listener: register takes callback function as input but unregister requires string. Other sdk's take callback function as input for both register and unregister listner #65

Closed sagarsshah closed 6 months ago

sagarsshah commented 6 months ago

I wanted to discuss a potential overhead to our rust SDK's listener apis. Currently, the register_listener() function within the SDK requires a listener parameter of type Box<dyn Fn(Result<UMessage, UStatus>) + Send + Sync + 'static>, returning a listener string for later use with unregister_listener(). However, I've found it challenging to manage listeners effectively without associating them directly with a UUri and associated function. Other SDKs like Java and Python, which accept the listener function directly for both registration and unregistration, our current Rust SDK requires additional bookkeeping to track listeners.

Though in real life scenario same uE will rarely listen twice on same UUri but still this can be good correction. Either we expose functions to convert listener function to listener string to uE developer OR we modify rust SDK to match other SDKs.

PLeVasseur commented 6 months ago

Hey @sagarsshah :wave:

Thanks for your feedback. I'll make a strawman proposal PR for us to all look at.

PLeVasseur commented 6 months ago

Hi @sagarsshah -- check out the draft PR here: #67

PLeVasseur commented 6 months ago

I took a different approach over in #68 that is a lot cleaner.

PLeVasseur commented 6 months ago

Latest attempt is in #69, using instance equality instead of type-level equality to align with what up-client-android-java is doing (and honestly, which makes more sense).

sagarsshah commented 6 months ago

Thanks. let me try #69 .

sagarsshah commented 6 months ago

I found integration to be very convenient, although I haven't tested it yet. I'll soon share my up-rust-socket code with you for your preview. Thanks.

PLeVasseur commented 6 months ago

Hey @sagarsshah -- we still need to get #69 reviewed before I want you to go too far in using it :slightly_smiling_face:

Let's wait a little longer on others to review the PR

PLeVasseur commented 6 months ago

Hey again @sagarsshah -- this was implemented in #69.

If this meets your needs, could you close the issue?

sagarsshah commented 6 months ago

I've completed the integration with my rust socket transport, and it seems to be ok. However, I still need to conduct testing to ensure everything is working as expected. hopefully by this weekend I will close this issue.

PLeVasseur commented 6 months ago

I've completed the integration with my rust socket transport, and it seems to be ok. However, I still need to conduct testing to ensure everything is working as expected. hopefully by this weekend I will close this issue.

I think this issue regarding adding the functionality requested could be closed if #69 meets your needs.

If you have another issue (bug or missing functionality) uncovered when testing up-rust you could open another issue.

WDYT?