eclipse-zenoh / zenoh-c

C API for Zenoh
http://zenoh.io
Other
80 stars 57 forks source link

take operation accepts mutable loaned reference for types which supports it #718

Open milyin opened 1 month ago

milyin commented 1 month ago

This update adds z_sample_take_loaned, z_reply_take_loaned, z_query_take_loaned and z_hello_take_loaned operations on corresponding z_loaned_sample_t*, z_loaned_reply_t*, z_loaned_hello_t*, and z_loaned_query_t*.

This API this is made for callbacks, which accepts data as mutable loaned references. Now user can either access the object on place or take it with z_xxx_take_loaned to process later.

Internally this update provides guaranty that for objects supporting take_loaned operations the reference to Option<...> object is converted to mutable z_loaned_xxx* pointer. I.e. the &Sample in converted to const z_loaned_sample* and &mut Option<Sample> - to z_loaned_sample_t*.

github-actions[bot] commented 1 month ago

PR missing one of the required labels: {'bug', 'internal', 'breaking-change', 'enhancement', 'dependencies', 'documentation', 'new feature'}

github-actions[bot] commented 1 month ago

PR missing one of the required labels: {'dependencies', 'new feature', 'breaking-change', 'enhancement', 'bug', 'documentation', 'internal'}

github-actions[bot] commented 3 weeks ago

PR missing one of the required labels: {'breaking-change', 'bug', 'dependencies', 'new feature', 'enhancement', 'internal', 'documentation'}

github-actions[bot] commented 3 weeks ago

PR missing one of the required labels: {'enhancement', 'internal', 'new feature', 'breaking-change', 'bug', 'dependencies', 'documentation'}

milyin commented 3 weeks ago

After quick discusion it was decided to reduce this update only for types actually used by callbacks

milyin commented 2 weeks ago

Agreed with @DenisBiryukov91 opition that the real goal is to allow user to take z_owned_bytes_t from the sample or query or reply. It's safer to just add functionality for that, z_take_loaned is not necessary in this case anymore

milyin commented 1 week ago

Summary of latest discussion with @DenisBiryukov91 about the issue:

The "take from loaned" functionality is necessary for C++

Explanation: C++ doesn't actually use "z_xxx_loaned_t*" types. Each C++ object is a wrapper over corresponding "owned" zenoh-c object. This works only for objects which conforms condition sizeof(z_xxx_loaned_t)==sizeof(z_xxx_owned_t) which is true for most of zenoh-c objects (except e.g. mutexes, which are not wrapped by C++).

Internally in rust the z_owned_xxx_t is typically Option<XXX> and z_loaned_xxx_t is just XXX. When the Option uses Null Pointer Optimization (https://doc.rust-lang.org/std/option/index.html) the transmute from XXX to Option<XXX> is allowed, and therefore the pointer conversion from z_moved_xxx_t to z_loaned_xxx_t is allowed.

The problem is in conversion in opposite direction: from loaned to moved. The example of problematic sequence of operations is:

The proposed solution: allow C++ to implement move constructor by taking important data from mutable loaned reference, keeping at the original place something "empty": dummy but valid. Thus the following operations should be defined:

  1. construction: parameters -> fills new z_owned_xxx_t
  2. const loan z_xxx_loan: z_owned_xxx_t -> const z_loaned_xxx_t*
  3. mutable loan z_xxx_loan_mut: z_owned_xxx_t* -> z_loaned_xxx_t*
  4. move z_xxx_move : z_owned_xxx_t* -> z_moved_xxx_t*
  5. take z_xxxx_take: z_moved_xxx_t* -> fills new z_owned_xxx_t, sets original z_owned_xxx_t to so-called "gravestone" state
  6. drop z_xxx_drop: z_moved_xxx_t* -> destruct original z_owned_xxx_t, set it to gravestone state
  7. new take from loaned: z_loaned_xxx_t* -> fills new z_owned_xxx_t, sets original z_owned_xxx_t to "unspecified" state.

The difference between "unspecified" and "gravestone" state is this: "Gravestone" is a state of owned object which corresponds to None in Rust for objects wrapped to Option. It doesn't contain any meaningful data, access to it may cause segmentation fault, drop operation on it not required, but safe. "unspecified" is a state which corresponds to some valid value in Rust (for Option-wrapped objects is always Some<...>). It maybe safe to check it's state, if such operation is provided (like e.g. z_bytes_len). Drop operation on it is required.


In short: new z_xxx_take_from_loaned(z_owned_xxx_t* dst, z_loaned_xxx_t* src) operation is added for some objects.