implement z_clone generic for types that support clone.

[DenisBiryukov91]

Describe the release item

Implement z_clone generic for types that support clone. Similarly to how it is done in pico.

[milyin]

from #644:

In zenoh-pico *_clone methods exists by default for all owned/loaned types, in zenoh-c not, so we missed some of them

• z_hello_clone
• z_keyexpr_clone
• z_publisher_clone
• z_queryable_clone
• z_reply_err_clone
• z_string_array_clone
• z_subscriber_clone

[DenisBiryukov91]

I'm not sure we should blindly allow clone on everything, since making for example clone of session/subscriber/publisher would likely allow to close/undeclare them from another thread resulting in unexpected behavior.

[milyin]

I'm not sure we should blindly allow clone on everything, since making for example clone of session/subscriber/publisher would likely allow to close/undeclare them from another thread resulting in unexpected behavior.

As far as I see in rust all these types are clonable. So there is no technical obstacle to implement clone in zenoh-c.
Dropping the object should not cause undeclaration after "background" feature merging, so dropping is not a problem at all.
The question is what happens when one thread undeclares publisher for example, while another thread sends data through it. But this can happen in Rust too, I suppose and this is handled somehow

[DenisBiryukov91]

This is not about publisher, since it does not seem to have any mutable methods, but more about subscriber. Subscriber clone would allow to undeclare it (effect visible in all other clones) - i.e. mutate something from a const pointer in C, or const ref in C++ - which really does not coincide with overall expectation that const pointers/refs do not allow mutable access.
So function signature f(z_loaned_subscriber_t* s) would provide 0 guarantees regarding s's mutability (and in this context it becomes totally unclear why we even bothered with introduction of move/loan etc, given that we are completely destroying the only safety mechanism existing in C with our clone implementation)

The other question is what value such clones bring to c/c++ user, given that everything it provides for subscriber can be achieved just by using pointer/ref to it in a controlled manner (i.e. by properly respecting const semantics).

[DenisBiryukov91]

The clones were introduced for reply/sample/query to allow moving them out of callbacks. Given that we now have proper move semantics with z_move, z_take, and that reply/sample/query are being passed by value in corresponding rust callbacks, may be it is worth considering returning them as owned pointers in the c-callbacks and get rid of questionable clones altogether ? The user might still have a choice to take or not ownership of owned object through z_move-z_take in case for example he does not want to make an extra call to z_drop.

[milyin]

The goal of passing loaned value to callback was to allow user to not care about passed parameter and therefore prevent memory leaks. So current solution is:

void query_handler(const z_loaned_query_t *query, void *context) { ... }

If we change it to

void query_handler(z_moved_query_t *query, void *context) { ... }

the only way to access the query will be through z_take and then user will be obliged to drop the query.

If we change it to

void query_handler(z_owned_query_t *query, void *context) { ... }

it is not clear what to do with this query because it's against our contract. Currently we pass pointer to owned object only to constructors, to initialize them.
(that's why I proposed z_uninit_xxx_t* type for constructors).

[milyin]

So I think the approach with "clone" is ok.
Clones exists in pico, clones exists in rust. I see absolutely no reason to not implement them in zenoh-c just to keep some const pointer purity. Yes, if you clone some complex object like subscriber, the clone may still have some state shared with the original and therefore the clone may affect the original. That's ok I think.

[DenisBiryukov91]

That's ok I think.

This is definitely not ok, since this breaks language invariant: the function taking const pointer/ref or const method are not supposed to cause any observable state modification of the argument.

I would rather go for void query_handler(z_owned_query_t *query, void *context) { ... } and either implement z_xxx_uninit since it now clearly becomes justified or just write in the documentation what z_owned_xxx_t* means in the context. Passing z_moved_xxx is technically also an ok option (although too cumbersome) since this is exactly how the query is being passed in rust (and also how it will be passed in c++ - i.e. via rvalue reference &&).

[milyin]

After live discussion the following decision was made:

• implement z_take methods for mutably loaned types z_loan_xxx_t*
• pass z_loan_xxx_t* to callbacks (instead of const z_load_xxx_t*), allowing user to take ownership of the owned object by z_take or keep ownership for caller. This will make clone unnecessary for callbacks.
• (to be discussed and coordinated with pico) disable clone either for undeclarable entities only or for all entities which have shallow clone in Rust

Separate tasks are created for this, links in eclipse-zenoh/zenoh#1372