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

implement protection type (z_moved_type_t, probably z_uninit_type_t) #396

Closed
milyin opened this issue May 28, 2024 · 2 comments
Closed

implement protection type (z_moved_type_t, probably z_uninit_type_t) #396

milyin opened this issue May 28, 2024 · 2 comments
Labels
api fix Problem in the API release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented May 28, 2024

The library uses concept of owned and loaned types.

The owned type z_owned_type_t it the object itself. The "owned" structure have the following restrictions:

  • it should not have multiple copies as it usually contains reference to external memory/socket/etc. and control it's state
  • it should be released at the end of it's lifecycle with z_drop_type function
  • it doesn't contain self-references so it can be safely moved to other place in memory
  • it have "empty" state returned by z_null_type function. Structure in empty state need not to be dropped, though it's safe to call drop on it

Operations on the owned structure are performed with pointer to special type z_loaned_type_t. This type is returned by functions

const z_loaned_type_t* z_loan_type(const z_owned_type_t*);
z_loaned_type_t* z_loan_type(z_owned_type_t*);

If some function accepts pointer to z_loaned_type, it's guaranteed that corresponding owned type is not consumed, i.e. the user still have to call z_drop_type on it.

If function accepts pointer to owned type itself, this means that it takes ownership of the object. It should leave the passed object in empty state to avoid double drop or memory leak. Example:

z_owned_config_t config;
z_config_new(&config);
z_owned_session_t session;
z_open(&session, &config);
// config here is consumed, no need to call z_drop_config()
z_get(z_loan_session(session), "foo/bar", ...);
z_drop_session(&session);

There is a macro z_move which just takes a pointer to object. I.e. here is the correct way to write the code above:

z_owned_config_t config;
z_config_new(&config);
z_owned_session_t session;
z_open(&z_session, z_move(config));
...
z_drop_session(z_move(session));

This approach have 2 problems:

  • prototype of "constructor" function (z_config_new) and prototype of function, taking ownership (z_open) are the same: both accepts pointer to z_owned_type_t. Though the semantic is different. z_config_new expects uninitialized memory or object in empty state. It doesn't care about current content of this memory. z_open requires initialized object (in some cases empty state can be allowed too, e.g. for optional parameters)
  • nothing forces developer to use this z_move macro. But without this macro the code becomes much less clean

Solution

The solution is to introduce two addtional wrapper types z_uninit_type_t and z_moved_type_t and corresponding functions to get these types. Normally no function should accept the pointer to owned object itself because it is ambigious. Instead the constructors should accept pointer to z_unitit_type_t and functions which takes ownership should accept z_moved_type_t.

The example above will look like this in this case:

z_owned_config_t config;
z_config_new(z_uninit_config(&config));
z_owned_session_t session;
z_open(z_uninit_session(&session), z_move_config(&config));
z_get(z_loan_session(&session), "foo/bar", ...);
z_drop_session(z_move_session(&session));

or more short with macros

z_owned_config_t config;
z_config_new(z_uninit(config));
z_owned_session_t session;
z_open(z_uninit(session), z_move(config));
z_get(z_loan(session), "foo/bar", ...);
z_drop_session(z_move(session));
@milyin milyin added the release Part of the next release label May 28, 2024
@milyin milyin added the api fix Problem in the API label May 30, 2024
@milyin
Copy link
Contributor Author

milyin commented May 30, 2024

Suggesstion: do it in 2 steps, move is necessary, uninit is tentative
zenoh-pico - does it cause any problems?

@milyin milyin changed the title implement protection types z_uninit_type_t, z_moved_type_t implement protection type z_moved_type_t Jun 9, 2024
@milyin milyin changed the title implement protection type z_moved_type_t implement protection type (z_moved_type_t, probably z_uninit_type_t) Jun 10, 2024
@milyin milyin moved this to In progress in Zenoh 1.0.0 release Jun 30, 2024
@milyin
Copy link
Contributor Author

milyin commented Aug 9, 2024

TODO:
zenoh-c branch #488
zenoh-pico branch eclipse-zenoh/zenoh-pico#575
zenoh-cpp branch - TODO

zenoh-c:

  • add test from Pico z_api_null_drop_test

zenoh-pico

zenoh-cpp
???

@milyin milyin closed this as completed Aug 29, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Zenoh 1.0.0 release Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api fix Problem in the API release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

1 participant