Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

milyin
Copy link
Contributor

@milyin milyin commented Sep 26, 2024

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*.

Copy link

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

Copy link

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

Copy link

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

Copy link

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

@milyin milyin added enhancement New feature or request release Part of the next release labels Oct 22, 2024
@milyin milyin marked this pull request as draft October 24, 2024 15:56
@milyin
Copy link
Contributor Author

milyin commented Oct 24, 2024

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

@milyin milyin marked this pull request as ready for review October 27, 2024 21:42
@milyin milyin marked this pull request as draft October 28, 2024 14:41
@milyin
Copy link
Contributor Author

milyin commented Oct 28, 2024

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
Copy link
Contributor Author

milyin commented Nov 5, 2024

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:

  • zenoh-c executes callback with, e.g. z_loaned_sample_t* argument
  • the z_loaned_sample_t* is transmuted to z_owned_sample_t* and then to C++ object reference zenoh::Sample& for C++ callback
  • user constructs the sample inside the callback using move constructor: Sample s2(std::move(sample)). This leaves on place of original sample the None value (in Rust representation). But if originally on Rust side it was just Sample, not a Option<Sample>, this causes crash: None is not a valid value for Sample

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.

@milyin milyin marked this pull request as ready for review November 8, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant