adds second kuksa client in zenoh-kuksa-provider #10

eriksven · 2024-10-15T17:23:51Z

To avoid a deadlock between waiting for the kuksa subscription and sending messages on the same client, a second client gets initialized. In addition, some clippy warning get fixed.

eriksven · 2024-10-15T17:25:10Z

@max-grzanna what do you think?

eriksven · 2024-10-15T17:31:57Z

I ran into a deadlock where the task with the function publish_to_zenoh could not acquire the lock for the kuksa_client because it seems to be locked by the the other task with the handling_zenoh_subscription . There is most likely a better solution, but my quick fix here now, is now to instantiate a second kuksa_client.

The approach of locking the client whenever one actually wants to send something might fall short too, when the kuksa_client is occupied by waiting on the subscription in the publish_to_zenoh (currently line 115 in main.rs). Because we obviously do not want that the handling_zenoh_subscription has to wait for a new incoming messages before being able to acquire the lock to execute set_current_values when required.

max-grzanna · 2024-10-21T14:01:41Z

Instantiating a second client works for me. I can't think of a better option either.

max-grzanna

LGTM

lukasmittag

Minor comments

lukasmittag · 2024-10-21T16:03:48Z

zenoh-kuksa-provider/Cargo.toml

@@ -12,6 +12,8 @@
 ########################################################################


+[workspace]
+


lukasmittag · 2024-10-21T16:09:48Z

zenoh-kuksa-provider/src/main.rs

@@ -123,7 +121,7 @@ async fn publish_to_zenoh(
                            let vss_path = &entry.path;

                            if let Some(publisher) = publishers.get(vss_path.as_str()) {
-                                let buf = match datapoint_to_string(&datapoint) {
+                                let buf = match datapoint_to_string(datapoint) {


please keep using the reference

lukasmittag · 2024-10-21T16:23:28Z

I think dropping the sub_client in the handle_zenoh_subscription should solve the issue already. https://fongyoong.github.io/easy_rust/Chapter_43.html If thats not the case please let me know

lukasmittag · 2024-10-21T16:32:01Z

Okay, might not solve it entirely since you're blocking with a while loop after subscribing to databroker. As you described not what you want yes so keep using two clients in this case. Nonetheless some comments about the implementation.

lukasmittag · 2024-10-21T16:34:55Z

zenoh-kuksa-provider/src/main.rs

@@ -182,7 +180,8 @@ async fn main() {

    let uri = kuksa::Uri::try_from(provider_config.kuksa.databroker_url.as_str())
        .expect("Invalid URI for Kuksa Databroker connection.");
-    let client = Arc::new(Mutex::new(kuksa::Client::new(uri)));
+    let client = Arc::new(Mutex::new(kuksa::Client::new(uri.clone())));
+    let actuation_client = kuksa::Client::new(uri);

    fetch_metadata(


since you await this function you do not need to have a

let client = Arc::new(Mutex::new(kuksa::Client::new(uri.clone())));

lukasmittag · 2024-10-21T16:35:32Z

zenoh-kuksa-provider/src/main.rs

@@ -182,7 +180,8 @@ async fn main() {

    let uri = kuksa::Uri::try_from(provider_config.kuksa.databroker_url.as_str())
        .expect("Invalid URI for Kuksa Databroker connection.");
-    let client = Arc::new(Mutex::new(kuksa::Client::new(uri)));
+    let client = Arc::new(Mutex::new(kuksa::Client::new(uri.clone())));


let client = kuksa::Client::new(uri.clone());

lukasmittag · 2024-10-21T16:36:05Z

zenoh-kuksa-provider/src/main.rs

@@ -182,7 +180,8 @@ async fn main() {

    let uri = kuksa::Uri::try_from(provider_config.kuksa.databroker_url.as_str())
        .expect("Invalid URI for Kuksa Databroker connection.");
-    let client = Arc::new(Mutex::new(kuksa::Client::new(uri)));
+    let client = Arc::new(Mutex::new(kuksa::Client::new(uri.clone())));
+    let actuation_client = kuksa::Client::new(uri);

    fetch_metadata(
        client.clone(),


use a reference here instead

lukasmittag · 2024-10-21T16:36:47Z

zenoh-kuksa-provider/src/main.rs

        }
    }
 }

 async fn publish_to_zenoh(
    provider_config: Arc<ProviderConfig>,
    session: Arc<Session>,
-    kuksa_client: Arc<Mutex<kuksa::Client>>,
+    mut kuksa_client: kuksa::Client,


use reference. mut not needed since you're not editing the client

I think it is necessary to have a mutable client because the implementation for the functions requires a mutable reference for self.

pub async fn subscribe_target_values( &mut self, paths: Vec<&str>,

Correct me if I'm wrong.

Will check if mut is necessary there, but yes you're right. Nice catch!

I've checked it is necessary so all good here

lukasmittag · 2024-10-21T16:37:13Z

zenoh-kuksa-provider/src/main.rs

@@ -55,8 +55,6 @@ async fn handling_zenoh_subscribtion(
 ) {


use reference for kuksa_client

eriksven · 2024-10-25T20:09:16Z

@lukasmittag I pushed a new commit, could you please have another look at some time. Sorry for the delay, I was on business trip this week.

lukasmittag · 2024-10-30T14:02:13Z

zenoh-kuksa-provider/src/main.rs

        }
    }
 }

 async fn publish_to_zenoh(
    provider_config: Arc<ProviderConfig>,
    session: Arc<Session>,
-    kuksa_client: Arc<Mutex<kuksa::Client>>,
+    mut kuksa_client: kuksa::Client,


I've checked it is necessary so all good here

lukasmittag · 2024-10-30T14:04:50Z

zenoh-kuksa-provider/src/utils/kuksa_utils.rs

    paths: Vec<&str>,
    metadata_store: &super::metadata_store::MetadataStore,
-) {
-    let mut client = kuksa_client.lock().await;
+) -> Client {


you should not return anything here right?

lukasmittag · 2024-10-30T14:05:04Z

zenoh-kuksa-provider/src/utils/kuksa_utils.rs

@@ -40,6 +39,8 @@ pub async fn fetch_metadata(
            },
        );
    }
+
+    kuksa_client


unnecessary to return this

lukasmittag · 2024-10-30T14:05:41Z

zenoh-kuksa-provider/src/main.rs

-    fetch_metadata(
-        client.clone(),
+    client = fetch_metadata(
+        client,


should work if you give a reference or clone the client

To avoid a deadlock between waiting for the kuksa subscription and sending messages on the same client, a second client gets initialized. In addition, some clippy warning get fixed.

…uksa-provider

lukasmittag

good enough to be merged. Note: TODO - check if mutable is really necessary in rust lib of kuksa

eriksven mentioned this pull request Oct 18, 2024

adds initial components and deployment and architecture diagram eclipse-sdv-blueprints/service-to-signal#2

Merged

5 tasks

max-grzanna approved these changes Oct 21, 2024

View reviewed changes

max-grzanna reviewed Oct 21, 2024

View reviewed changes

lukasmittag requested changes Oct 21, 2024

View reviewed changes

lukasmittag reviewed Oct 30, 2024

View reviewed changes

eriksven added 3 commits November 4, 2024 10:50

adds second kuksa client in zenoh-kuksa-provider

34cefd0

To avoid a deadlock between waiting for the kuksa subscription and sending messages on the same client, a second client gets initialized. In addition, some clippy warning get fixed.

decreases the part for when the kuksa client is locked in the zenoh-k…

501d510

…uksa-provider

removes mutexes from zenoh-kuksa-client

62f4012

eriksven force-pushed the zenoh-provider-two-clients branch from e9af139 to 62f4012 Compare November 4, 2024 09:50

lukasmittag approved these changes Nov 4, 2024

View reviewed changes

lukasmittag merged commit 1cb785f into eclipse-kuksa:main Nov 4, 2024
4 checks passed

eriksven mentioned this pull request Nov 4, 2024

upgrades commit for zenoh-kuksa-provider eclipse-sdv-blueprints/service-to-signal#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds second kuksa client in zenoh-kuksa-provider #10

adds second kuksa client in zenoh-kuksa-provider #10

eriksven commented Oct 15, 2024

eriksven commented Oct 15, 2024

eriksven commented Oct 15, 2024

max-grzanna commented Oct 21, 2024

max-grzanna left a comment

lukasmittag left a comment •

edited

Loading

lukasmittag Oct 21, 2024

lukasmittag Oct 21, 2024

lukasmittag commented Oct 21, 2024

lukasmittag commented Oct 21, 2024

lukasmittag Oct 21, 2024

lukasmittag Oct 21, 2024

lukasmittag Oct 21, 2024

lukasmittag Oct 21, 2024

max-grzanna Oct 25, 2024

lukasmittag Oct 28, 2024

lukasmittag Oct 30, 2024

lukasmittag Oct 21, 2024

eriksven commented Oct 25, 2024

lukasmittag Oct 30, 2024

lukasmittag Oct 30, 2024

lukasmittag Oct 30, 2024

lukasmittag Oct 30, 2024

lukasmittag left a comment

		@@ -12,6 +12,8 @@
		########################################################################


		[workspace]

adds second kuksa client in zenoh-kuksa-provider #10

adds second kuksa client in zenoh-kuksa-provider #10

Conversation

eriksven commented Oct 15, 2024

eriksven commented Oct 15, 2024

eriksven commented Oct 15, 2024

max-grzanna commented Oct 21, 2024

max-grzanna left a comment

Choose a reason for hiding this comment

lukasmittag left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasmittag commented Oct 21, 2024

lukasmittag commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriksven commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasmittag left a comment

Choose a reason for hiding this comment

lukasmittag left a comment •

edited

Loading