refactor: wrap the zenoh session with a shared pointer

[YuanYuYuan]

Closes ZettaScaleLabs#44.

[clalancette]

I generally like the idea here, but I think we need to do some more work particularly in the ClientData class. I've left a detailed proposal in there.

[ahcorde]

These changes are already included in the zenoh_c to zenoh_cpp PR https://github.com/ros2/rmw_zenoh/pull/327/files#diff-124e1086db3e5f3edb76619bd65943a900a963ef671e014c817d3bf27d803908

[clalancette]

These changes are already included in the zenoh_c to zenoh_cpp PR https://github.com/ros2/rmw_zenoh/pull/327/files#diff-124e1086db3e5f3edb76619bd65943a900a963ef671e014c817d3bf27d803908

I do think we should get this one in first, and then go for the switch to zenoh-cpp. That will allow us to concentrate on one thing at a time, and I think this one is largely ready to go (with my one big comment up above still to be addressed).

[YuanYuYuan]

These changes are already included in the zenoh_c to zenoh_cpp PR https://github.com/ros2/rmw_zenoh/pull/327/files#diff-124e1086db3e5f3edb76619bd65943a900a963ef671e014c817d3bf27d803908

I do think we should get this one in first, and then go for the switch to zenoh-cpp. That will allow us to concentrate on one thing at a time, and I think this one is largely ready to go (with my one big comment up above still to be addressed).

Hi @clalancette, I'm trying to understand the underlying issue with the client and the request-in-flight. But I'm wondering why we must clone a session for each request. It seems to me either the callback or the client_data_drop doesn't utilize the session.

[clalancette]

Hi @clalancette, I'm trying to understand the underlying issue with the client and the request-in-flight. But I'm wondering why we must clone a session for each request. It seems to me either the callback or the client_data_drop doesn't utilize the session.

Doesn't client_data_drop implicitly use the session, by the fact that it being called by the internals of Zenoh? If that is not the case, then indeed we do not need to hold onto this here. But in that case, I think we would want to hold onto the session for the lifetime of ClientData (i.e. take the reference in the constructor).

[YuanYuYuan]

Doesn't client_data_drop implicitly use the session, by the fact that it being called by the internals of Zenoh? If that is not the case, then indeed we do not need to hold onto this here. But in that case, I think we would want to hold onto the session for the lifetime of ClientData (i.e. take the reference in the constructor).

1. The client_data_drop is part of the closure. It doesn't really depend on a zenoh session but a zenoh runtime which is global and internal.
2. But whether the callback is triggered or not makes a difference. I ran some quick tests and discovered that in the recent zenoh version, a get request sent to a remote queryable won't trigger the callback if the get session is closed between the time the queryable receives the request and sends back the reply. The question is do we want to make sure each reply is handled even after the session is closed? If the answer is yes, then we need to clone the session for every request. Otherwise, we don't.
3. We recently introduced the Querier, which works like a session.put vs publisher. This means we can keep the Querier rather than a session as we do for the other rmw data in the future.

[clalancette]

• But whether the callback is triggered or not makes a difference. I ran some quick tests and discovered that in the recent zenoh version, a get request sent to a remote queryable won't trigger the callback if the get session is closed between the time the queryable receives the request and sends back the reply. The question is do we want to make sure each reply is handled even after the session is closed? If the answer is yes, then we need to clone the session for every request. Otherwise, we don't.

No, I don't think we need to handle the reply after the session is closed. In our case, the session being closed is the result of the context being shutdown, and after that point nothing in the higher layers can handle anything new anyway.

OK, so in that case, I'm going to withdraw my previous comment about fixing it (though I will open a separate PR doing that for the ClientData, which does need to be kept alive). I'll put a different comment about possibly moving the Session reference count elsewhere.

[clalancette]

One more change to make, then I think this is good to go.

[clalancette]

I don't think we need this, right? Because we are holding onto the session while this ClientData is alive, this is an unnecessary additional reference count, and I think it is enough to just keep this in the constructor.

[YuanYuYuan]

Thanks. It's misplaced. Resolved in 416f430

[YuanYuYuan]

Rebase it against the latest rolling.