-
Notifications
You must be signed in to change notification settings - Fork 43
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
Zenoh 1.0.0 Porting #276
Zenoh 1.0.0 Porting #276
Conversation
Signed-off-by: Luca Cominardi <[email protected]>
Signed-off-by: Luca Cominardi <[email protected]>
Signed-off-by: Luca Cominardi <[email protected]>
chore: checkout dev/1.0.0
…n the zenoh_c_vendor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed 75% of the changes and provided some feedback. My primary concern is the introduction of registering a function with the atexit
signal handler. As explained inline, there is still a problem of the static variable being invalidated and i'm confused why we need it with dev/1.0.0 when we do not need it rolling.
Secondly, I think the diff in the PR is way larger than it needs to be. Imo the changes only need reflect changes in the zenoh-c API (and things like attachment_helpers) but there are changes in rmw_zenoh.cpp wrt to checking if entities are valid, zenoh utils, and various cleanups to headers.
My recommendation is to take all the changes that remove cleanup headers and target a PR to the current rolling
branch. And save other changes unrelated to breaking zenoh-c API for a future PR.
@@ -361,28 +351,21 @@ rmw_ret_t ClientData::take_response( | |||
} | |||
|
|||
// Fill in the request_header | |||
request_header->request_id.sequence_number = | |||
rmw_zenoh_cpp::get_int64_from_attachment(&sample->attachment, "sequence_number"); | |||
rmw_zenoh_cpp::attachment_data_t attachment(z_sample_attachment(sample)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API feels awkward and requires you to define the move constructor. Why not just pass const z_loaned_sample_t *
and construct the z_sample_attachment
internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to miss this. The attachment is nothing but the usual zenoh bytes in 1.0.
On the other hand, zenoh-c basically doesn't provide any special structure for the users.
That's why we need to define an attachment data type and its (de)serialization.
The rmw_zenoh_cpp::attachment_data_t
in this PR is considered to mapped to a z_bytes. One of the constructor is defined as
attachment_data_t::attachment_data_t(const z_loaned_bytes_t * attachment)
{
...
}
which means we can parse the given attachment from raw zenoh bytes to the desired data types.
We also use this pattern in zenoh-cpp.
auto attachment = sample.get_attachment();
if (!attachment.has_value()) return;
// Assume we convert the raw bytes to a `std::unordered_map<std::string, std::string>`
auto attachment_data = ext::deserialize<std::unordered_map<std::string, std::string>>(attachment->get());
if (zc_liveliness_token_check(&token_)) { | ||
zc_liveliness_undeclare_token(z_move(token_)); | ||
} | ||
if (z_check(z_loan(keyexpr_))) { | ||
z_drop(z_move(keyexpr_)); | ||
} | ||
zc_liveliness_undeclare_token(z_move(token_)); | ||
|
||
z_drop(z_move(keyexpr_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We previously needed this check since it was possible for token_
and keyexpr_
to be uninitialized when shutdown is called within ClientData::make
. Do these functions now check whether the tokens are valid before dropping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is one of the misunderstandings we would like to avoid.
- There's no way to check an uninitialized zenoh entity.
z_owned_keyexpr_t keyexpr;
z_check(z_loan(keyexpr)) // This is an undefined behavior.
A proper way to use z_check
is to use it on an initialized data.
z_owned_keyexpr_t keyexpr;
z_keyexpr_from_str(&keyexpr, "foo");
z_check(z_loan(keyexpr)); // This is okay.
z_owned_keyexpr_t keyexpr;
z_null(&keyexpr);
z_check(z_loan(keyexpr)); // This is also fine.
-
The check itself checks the gravestone status of zenoh data. It doesn't do functional checks like liveliness token querying over the network. These functions are necessary for zenoh-cpp development yet are easy to misuse. That's why we finally decided to make them internal. See
z_check
andz_null
made internal eclipse-zenoh/zenoh-c#605 for details. -
Regarding the contention issue you mentioned, I assume that the function
ClientData::shutdown
can only be called once a valid ClientData instance has been created. However, if we prefer to relax this restriction, we could introduce a flag to indicate whether the class object has been properly validated. This would help address the issue. What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining that. Right now ClientData::shutdown
can be invoked even if the tokens are not properly initialized. So it sounds like we would need to introduce an initialized_
flag to the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've uniformly applied the initialized_
flag on all the detail/rmw_*_data
in ZettaScaleLabs@4c63e8d
rmw_zenoh_cpp/src/rmw_zenoh.cpp
Outdated
if (!context_impl->session_is_valid()) { | ||
RMW_SET_ERROR_MSG("zenoh session is invalid"); | ||
return nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short answer: the z_session_check
function has been removed. The z_check
function has been changed to internal in eclipse-zenoh/zenoh-c#605. The check itself is to check the gravestone health of the zenoh data. If we want to check whether the session is constructed successfully, we should use the return code during the creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say the session was constructed successfully but was closed later on. How would we check if the session is still open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a function z_session_is_closed
and it's been used as the implementation of rmw_context_impl_s::Data session_is_valid
. I think I can add it back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ZettaScaleLabs@09b45c3.
rmw_zenoh_cpp/src/rmw_zenoh.cpp
Outdated
if (!pub_data->liveliness_is_valid()) { | ||
return RMW_RET_ERROR; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated above, the z_check
has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ZettaScaleLabs@09b45c3.
Addressed in ZettaScaleLabs@84d1267.
rmw_zenoh_cpp/src/rmw_zenoh.cpp
Outdated
if (!context_impl->session_is_valid()) { | ||
RMW_SET_ERROR_MSG("zenoh session is invalid"); | ||
return nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why remove this here and in other functions below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated above, the z_check
has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ZettaScaleLabs@09b45c3.
* Fix `z_view_string_t` to `std::string` conversion * Fix formatting
Latency comparison
|
|
Follow-up on Review Feedback
z_owned_publisher_t pub;
auto free_type_hash_c_str = rcpputils::make_scope_exit([&]() {
z_drop(z_move(pub));
});
if (z_declare_publisher(session, &pub, ...) != Z_OK) {
return;
} Since we actually don't need to z_owned_publisher_t pub;
if (z_declare_publisher(session, &pub, ...) != Z_OK) {
return;
}
auto free_type_hash_c_str = rcpputils::make_scope_exit([&]() {
z_drop(z_move(pub));
});
(*) This means the change is held on the branch https://github.com/ZettaScaleLabs/rmw_zenoh/tree/wip/session-clone and not yet merged into dev/1.0.0. |
All right, so I just tried out the latest here (I'm sorry I haven't gotten to this earlier). First of all, I needed the following patch to compile:
After that, I tried to run the tests in
I also tried out the |
Ah, never mind, this was a local problem. Things are working OK now (though I still need the patch above). Will continue testing. |
{ | ||
std::lock_guard<std::recursive_mutex> lock(mutex_); | ||
return zc_liveliness_token_check(&token_); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that these z_check
functions have been made internal but having a method to check whether the liveliness token is still valid is useful. How about we simply return !is_shutdown_
here since we unregister the token only after the entity is shutdown. Then keep the function call in rmw_zenoh.cpp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we only check liveliness_is_valid
in rmw_zenoh_cpp/src/rmw_zenoh.cpp. Let me change it with is_shutdown()
there. ZettaScaleLabs@84d1267
rolling branch
rmw_zenoh_cpp/src/rmw_zenoh.cpp
754: if (!pub_data->liveliness_is_valid()) {
rmw_zenoh_cpp/src/detail/rmw_service_data.hpp
63: bool liveliness_is_valid() const;
rmw_zenoh_cpp/src/detail/rmw_service_data.cpp
242:bool ServiceData::liveliness_is_valid() const
rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp
431:bool PublisherData::liveliness_is_valid() const
rmw_zenoh_cpp/src/detail/rmw_publisher_data.hpp
74: bool liveliness_is_valid() const;
rmw_zenoh_cpp/src/detail/rmw_subscription_data.hpp
90: bool liveliness_is_valid() const;
rmw_zenoh_cpp/src/detail/rmw_client_data.cpp
281:bool ClientData::liveliness_is_valid() const
rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp
389:bool SubscriptionData::liveliness_is_valid() const
rmw_zenoh_cpp/src/detail/rmw_client_data.hpp
64: bool liveliness_is_valid() const;
The branch is somewhat out of sync with the wip/session-clone. I just pushed a fix with the typo fix. Thanks! |
New results
|
Signed-off-by: Yadunund <[email protected]>
Notes
z_view_keyexpr_t
ratherz_owned_keyexpr_t
if possible. This keeps the reference only and doesn't need to be dropped.z_check
is now internal. We check zenoh entities' health by the creation functions' return.