Fixes for services playback per review 2 #6

MichaelOrlov · 2024-03-31T21:01:04Z

Relates Support service 2/2 --- rosbag2 service play ros2/rosbag2#1481

Made the following changes per review:

Make service_event_ts_lib as private member again
- Motivation: The shared pointer to the service event type support library shall be a member variable to make sure that the library is loaded during the liveliness of the instance of this class, since we have raw pointers to its inner members.
Cleanup in PlayerServiceClient::async_send_request(ser_message)
- Rewrite raw pointers arithmetic. The assumption that size_t represents the size of the void* may not necessarily be true on all platforms.
- Use shared pointer with custom deleter for deserialized messages.
- The assumption that we can take the first element from the bounded sequence by dereferencing raw pointer to the bounded sequence may not necessarily be true and up to the underlying rmw and transport layer implementation. Use dedicated request_member.get_function(request_sequence_ptr, 0) function instead.
- Add sanity checks for service_event_members_ in PlayerServiceClient constructor.
Refactoring. Do full deserialization and only once
- Rationale: We can't rely on the assumption that we can safely partially deserialize service event to the ServiceEventInfo structure.

- Motivation: The shared pointer to the service event type support library shall be a member variable to make sure that library loaded during the liveliness of the instance of this class, since we have a raw pointers to its inner members. Signed-off-by: Michael Orlov <[email protected]>

- Rewrite raw pointers arithmetic. Assumption that size_t represent size of the void* may not necessarily be true on all platforms. - Use shared pointer with custom deleter for deserialized message. - Assumption that we can take first element from bounded sequence by dereferencing raw pointer to the bounded sequence may not be necessarily be true and up to the underlying rmw and transport layer implementation. Use dedicated request_member.get_function(request_sequence_ptr, 0) function instead. - Add sanity checks `for service_event_members_` in PlayerServiceClient constructor. Signed-off-by: Michael Orlov <[email protected]>

- Rationale: We can't rely on assumption that we can safely partially deserialize service event to the ServiceEventInfo structure. Signed-off-by: Michael Orlov <[email protected]>

MichaelOrlov · 2024-03-31T21:03:47Z

@Barry-Xu-2018 @fujitatomoya Here is the draft of my second round changes per review. I haven't done with this PR yet, but you can consider my changes and make some comments.
I will be on vacation during the next week. However, going to get back to this PR in the middle of the week.

fujitatomoya · 2024-04-01T04:00:59Z

@MichaelOrlov thanks we will be reviewing this early in this week.

Barry-Xu-2018 · 2024-04-01T06:22:07Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+      return false;
+    }
+
+    if (!service_client->is_service_event_include_request_message(service_event)) {


It appears to overlook the scenario where both the service and the client enable introspection contents for the same client ID.

Old function is PlayerServiceClient::is_include_request_message().
Except checking if request is included, this function also includes the following logic

Decide whether the request comes from the service or the client is used for one client ID. (Currently, it is decided by the first message including the request data. e.g. First message with request from client. We will use message from client to send request, even if message from service also provide request data.)

Consider the case. Service/client switched introspection type from metadata to contents in recorded data.
I don't consider the case. Service/Client switched introspection type from contents to metadata in record data, which leads that the decision in 1 maybe adjusted.

rosbag2/rosbag2_transport/src/rosbag2_transport/player_service_client.cpp

Lines 55 to 159 in a8312de

bool PlayerServiceClient::is_include_request_message(const rcl_serialized_message_t & message)

{

auto [type, client_id, sequence_number] = get_msg_event_type(message);

// Ignore response message

if (type == service_msgs::msg::ServiceEventInfo::RESPONSE_SENT ||

type == service_msgs::msg::ServiceEventInfo::RESPONSE_RECEIVED)

{

return false;

}

bool ret = false;

// For each Client, decide which request data to use based on the first message related to

// the request that is obtained from the record data.

// e.g.

auto iter = request_info_.find(client_id);

if (type == service_msgs::msg::ServiceEventInfo::REQUEST_RECEIVED) {

if (!service_set_introspection_content_) {

if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {

service_set_introspection_content_ = true;

}

}

if (iter != request_info_.end()) {

switch (iter->second) {

case request_info_from::CLIENT:

{

// Already decide using request data from client.

break;

}

case request_info_from::NO_CONTENT:

{

if (service_set_introspection_content_) {

// introspection type is changed from metadata to metadata + contents

request_info_[client_id] = request_info_from::SERVICE;

ret = true;

} else {

RCUTILS_LOG_WARN_ONCE_NAMED(

ROSBAG2_TRANSPORT_PACKAGE_NAME,

"The configuration of introspection for '%s' is metadata on service side !",

service_name_.c_str());

}

break;

}

default: // request_info_from::SERVICE:

{

// Already decide using request data from service.

ret = true;

}

}

} else {

if (service_set_introspection_content_) {

request_info_[client_id] = request_info_from::SERVICE;

ret = true;

} else {

request_info_[client_id] = request_info_from::NO_CONTENT; // Only have metadata

}

}

return ret;

}

// type is service_msgs::msg::ServiceEventInfo::REQUEST_SENT

if (iter != request_info_.end()) {

switch (iter->second) {

case request_info_from::CLIENT:

{

// Already decide using request data from client.

ret = true;

break;

}

case request_info_from::NO_CONTENT:

{

if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {

// introspection type is changed from metadata to metadata + contents

request_info_[client_id] = request_info_from::CLIENT;

ret = true;

} else {

RCUTILS_LOG_WARN_ONCE_NAMED(

ROSBAG2_TRANSPORT_PACKAGE_NAME,

"The configuration of introspection for '%s' client [ID: %s]` is metadata !",

rosbag2_cpp::client_id_to_string(client_id).c_str(),

service_name_.c_str());

}

break;

}

default: // request_info_from::SERVICE:

{

// Already decide using request data from service.

ret = false;

}

}

} else {

if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {

request_info_[client_id] = request_info_from::CLIENT;

ret = true;

} else {

request_info_[client_id] = request_info_from::NO_CONTENT;

}

}

return ret;

}

@Barry-Xu-2018 Thanks for finding and pointing out this. It seems I overlook these cases.
From one side I understand that it would be nice to have some autodetection algorithm to be able to decide whether to use a service request from the original client request or from the service request confirmation event message.
However, this sophisticated logic could have corner cases and would be difficult to understand or explain for final users.
In addition, it seems the original implementation with a "hidden" autodetection algorithm from the PlayerServiceClient::is_include_request_message() is not going to work due to the found issue on the RMW layer [service introspection event does not have unique GID during transaction
ros2#357](ros2/rmw#357).

I am more inclined to implement a simple logic for a while to allow publish service requests upon a boolean flag by client request or by service confirmation and add this flag to the settings and perhaps to the CLI or env variable. Need to figure out what is better on the current stage.

Also, I think that we need to publish original service events in all other cases to be able to see them in the topic echo during playback or other introspection tools.

cc: @fujitatomoya

I am more inclined to implement a simple logic for a while to allow publish service requests upon a boolean flag by client request or by service confirmation and add this flag to the settings and perhaps to the CLI or env variable. Need to figure out what is better on the current stage.

that sounds reasonable to me as well. in default, we could rely the service events on service server side. i think this is easier and suggested way to use, but not always we can do this with 3rd party components.

Also, I think that we need to publish original service events in all other cases to be able to see them in the topic echo during playback or other introspection tools.

I really do not see the benefit for this, can you share the use case for this?

i would do, ros2 bag play --service and enable introspection on server application to see the service server is working okay with ros2 service echo. if we publish all the service events on the topic at the same time during playback, that ros2 service echo would be really complicated and printing duplicated messages?

or are you suggesting that we would want to publish the service events with topic via specific option?

@MichaelOrlov

However, this sophisticated logic could have corner cases and would be difficult to understand or explain for final users.

Yes. Since users are not aware of the internal selection logic, it can lead to confusion about the result under certain scenario.

I am more inclined to implement a simple logic for a while to allow publish service requests upon a boolean flag by client request or by service confirmation and add this flag to the settings and perhaps to the CLI or env variable. Need to figure out what is better on the current stage.

I also agree with using simpler logic. However, this might result in losing support for complex scenarios, such as some services enabling introspection while others have client introspection enabled. It would require users to uniformly use either service introspection or client introspection.

I incline to add new option for play.
e.g.
--service-request-from {service, client}. service is default value.
If there's a need to add automatic selection in the future (This depends on whether there is a demand from the users.), we can add an "auto" option.

Also, I think that we need to publish original service events in all other cases to be able to see them in the topic echo during playback or other introspection tools.

Are you saying that users, besides being able to replay service requests, can also replay the service event topic like a normal topic, allowing other introspection tools to analyze recorded the information via service event topic?

If yes, enable this function, is there a new parameter for play (if this parameter exists, play service event topic instead of play service request) ? About filter parameters (--services, --exclude-services, etc), do they work? Or user should use --topics, --exclude-topics since service event topic is replayed as normal topic.)

About filter parameters (--services, --exclude-services, etc), do they work? Or user should use --topics, --exclude-topics since service event topic is replayed as normal topic.)

After checking current code, --services and --exclude-services should still function normally, affecting the playback of service events.

@Barry-Xu-2018 Ok I've cherry-picked (d6db1bb to support --publish-service-requests) and made some fixes above it. See my new commits on this branch.

@MichaelOrlov

I will update other service-related unit tests by this way.

I updated test code based on latest code. Please refer to e096323.

@Barry-Xu-2018 Thanks for updating tests in the e096323 I will review your changes at my Thursday morning or first half of the day.
Meanwhile could you please review my changes per this PR one more time?

@Barry-Xu-2018 I've cherry-picked your changes in tests from e096323. Honestly, didn't have time to review thoroughly, but this shouldn't be a problem.
If you don't have any more comments/requests for changes for This PR let's move forward and merge rhi PR to your branch.
I think we almost good to go. Will need to rebase and run CI after merge.

I have some nitpick in mind to fix - but we can do it after code freeze

Barry-Xu-2018 · 2024-04-01T06:40:17Z

@MichaelOrlov

Thank you for your revision. I have no further comments aside from the previous one.

fujitatomoya

overall looks good to me.

rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp

…ustify errors Signed-off-by: Barry Xu <[email protected]>

- Rationale: We are moving to the new version of the uncrustify in rolling and still haven't done it yet fully for the baseline. The discrepancy in style for other untouched files like logging.hpp shall be addressed in a separate PR. Signed-off-by: Michael Orlov <[email protected]>

ros2bag/ros2bag/verb/play.py

- Also rename enum class ServiceRequestFrom to the ServiceRequestsSource Signed-off-by: Michael Orlov <[email protected]>

- We need this API to be able to write deterministic and non-flaky tests Signed-off-by: Michael Orlov <[email protected]>

Signed-off-by: Michael Orlov <[email protected]>

- Get rid of timeout inside tests and use newly added player->wait_for_sent_service_requests_to_finish(service_name) API. Signed-off-by: Michael Orlov <[email protected]>

Signed-off-by: Michael Orlov <[email protected]>

Also added a new option publish_service_requests to the PlayOptions. Note: By default rosbag2 player will publish service events only. Signed-off-by: Barry Xu <[email protected]> Signed-off-by: Michael Orlov <[email protected]>

Co-authored-by: Barry Xu <[email protected]> Signed-off-by: Michael Orlov <[email protected]>

- Long story short: Make it deterministic and run fast. Signed-off-by: Michael Orlov <[email protected]>

Signed-off-by: Michael Orlov <[email protected]>

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-04-12T02:01:03Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+    auto service_event_name = rosbag2_cpp::service_name_to_service_event_topic_name(service_name);
+    auto client_iter = service_clients_.find(service_event_name);
+    if (client_iter != service_clients_.end()) {
+      is_requests_complete = client_iter->second->wait_for_sent_requests_to_finish(timeout);


Finally, it also will call PlayerServiceClientManager::wait_for_all_futures().
So this operation does not wait for the future of the client of the specified service to complete, but waits for all futures to complete.

@Barry-Xu-2018 In this particular case it will wait for all futures for specific client.
However if service_name is empty it will wait for all futures from all clients, - does it make sense?

@MichaelOrlov

However if service_name is empty it will wait for all futures from all clients, - does it make sense?

Yes. It makes sense.

In this particular case it will wait for all futures for specific client.

Although the code retrieves the corresponding client based on the service name, client->wait_for_sent_requests_to_finish() actually calls PlayerServiceClientManager::wait_for_all_futures(). This function does not wait for all futures of a specific client; instead, it waits for all futures of all registered clients.

Barry-Xu-2018 · 2024-04-12T02:09:26Z

rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp

+    if (topic_type.length() <= std::strlen(service_event_topic_type_postfix)) {
+      return false;
+    }


This code is unnecessary since this code is else part of line 46.

Barry-Xu-2018 · 2024-04-12T02:16:43Z

rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp

+    if (topic_name.substr(
+        topic_name.length() -
+        strlen(RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX)) !=
+      RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX)
+    {
+      return service_name;
+    }
+
+    service_name = topic_name.substr(
+      0, topic_name.length() - strlen(RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX));


Perhaps we could simplify these codes.

} else { if (topic_name.substr( topic_name.length() - strlen(RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX)) == RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX) { service_name = topic_name.substr( 0, topic_name.length() - strlen(RCL_SERVICE_INTROSPECTION_TOPIC_POSTFIX)); } return service_name; }

Barry-Xu-2018 · 2024-04-12T02:36:55Z

rosbag2_transport/src/rosbag2_transport/player.cpp

@@ -1221,10 +1221,8 @@ bool PlayerImpl::publish_message(rosbag2_storage::SerializedBagMessageSharedPtr

  // Try to publish message as service request
  auto client_iter = service_clients_.find(message->topic_name);
-  if (client_iter != service_clients_.end()) {
+  if (play_options_.publish_service_requests && client_iter != service_clients_.end()) {


I think play_options_.publish_service_requests is unnecessary.
In the process of PlayerImpl::prepare_publishers(), play_options_.publish_service_requests can make sure service event topic is in publishers_.
So if play_options_.publish_service_requests is not set, the service event topic is already handled in the code for publisher_.

@Barry-Xu-2018 I would keep it for defensive programming or if in the future one will decide to change the value of the play_options_.publish_service_requests in runtime.

Barry-Xu-2018 · 2024-04-12T02:53:18Z

@MichaelOrlov

I have a few minor comments.

If you don't have any more comments/requests for changes for This PR let's move forward and merge rhi PR to your branch.

I will merge this PR. And above minor comments, I will continue to do in ros2#1481
Thank you.

MichaelOrlov added 3 commits March 31, 2024 10:40

Refactoring. Do full deserialization and only once

6397df6

- Rationale: We can't rely on assumption that we can safely partially deserialize service event to the ServiceEventInfo structure. Signed-off-by: Michael Orlov <[email protected]>

Barry-Xu-2018 reviewed Apr 1, 2024

View reviewed changes

fujitatomoya reviewed Apr 1, 2024

View reviewed changes

rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp Outdated Show resolved Hide resolved

Barry-Xu-2018 and others added 2 commits April 8, 2024 22:23

Specify service request from which introspection message and fix uncr…

94d418f

…ustify errors Signed-off-by: Barry Xu <[email protected]>

MichaelOrlov commented Apr 9, 2024

View reviewed changes

ros2bag/ros2bag/verb/play.py Outdated Show resolved Hide resolved

MichaelOrlov and others added 11 commits April 9, 2024 13:27

Rename service_request_from to the service_requests_source

05a3d1d

- Also rename enum class ServiceRequestFrom to the ServiceRequestsSource Signed-off-by: Michael Orlov <[email protected]>

Add Player::wait_for_sent_service_requests_to_finish() API

9d98b04

- We need this API to be able to write deterministic and non-flaky tests Signed-off-by: Michael Orlov <[email protected]>

Mitigate potential issues related to the operations reordering on ARM

b7116b9

Signed-off-by: Michael Orlov <[email protected]>

Make tests play_service_requests_from_service(client) deterministic

b87c944

- Get rid of timeout inside tests and use newly added player->wait_for_sent_service_requests_to_finish(service_name) API. Signed-off-by: Michael Orlov <[email protected]>

Misc findings and improvements 1

bede854

Signed-off-by: Michael Orlov <[email protected]>

Rename get_services_clients() to the get_service_clients()

c5f6c13

Signed-off-by: Michael Orlov <[email protected]>

Fix an issue on filtering topic when prepare publishers

091620c

Co-authored-by: Barry Xu <[email protected]> Signed-off-by: Michael Orlov <[email protected]>

Cleanup in play_without_publish_service_requests

3c19873

- Long story short: Make it deterministic and run fast. Signed-off-by: Michael Orlov <[email protected]>

Wrap code which can throw with try-catch in the publish_message(..)

98c14ef

Signed-off-by: Michael Orlov <[email protected]>

Delete some part of the code which became absolute and shall not be used

f270ac1

Signed-off-by: Michael Orlov <[email protected]>

MichaelOrlov changed the title ~~(WIP) Fixes for services playback per review 2~~ Fixes for services playback per review 2 Apr 11, 2024

MichaelOrlov marked this pull request as ready for review April 11, 2024 09:07

Update test codes

974d687

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 reviewed Apr 12, 2024

View reviewed changes

Barry-Xu-2018 merged commit 089d326 into Barry-Xu-2018:review/rosbag2_service_play Apr 12, 2024
10 of 11 checks passed

Barry-Xu-2018 mentioned this pull request Apr 12, 2024

Support service 2/2 --- rosbag2 service play ros2/rosbag2#1481

Merged

MichaelOrlov deleted the morlov/fixes_for_services_playback_per_review_2 branch April 13, 2024 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for services playback per review 2 #6

Fixes for services playback per review 2 #6

MichaelOrlov commented Mar 31, 2024

MichaelOrlov commented Mar 31, 2024

fujitatomoya commented Apr 1, 2024

Barry-Xu-2018 Apr 1, 2024

MichaelOrlov Apr 5, 2024

fujitatomoya Apr 5, 2024

Barry-Xu-2018 Apr 7, 2024

Barry-Xu-2018 Apr 7, 2024

MichaelOrlov Apr 10, 2024

Barry-Xu-2018 Apr 11, 2024

MichaelOrlov Apr 11, 2024

MichaelOrlov Apr 11, 2024

MichaelOrlov Apr 11, 2024

Barry-Xu-2018 commented Apr 1, 2024

fujitatomoya left a comment

Barry-Xu-2018 Apr 12, 2024

MichaelOrlov Apr 12, 2024

Barry-Xu-2018 Apr 12, 2024

Barry-Xu-2018 Apr 12, 2024

Barry-Xu-2018 Apr 12, 2024

Barry-Xu-2018 Apr 12, 2024

MichaelOrlov Apr 12, 2024

Barry-Xu-2018 commented Apr 12, 2024 •

edited

Loading

	bool PlayerServiceClient::is_include_request_message(const rcl_serialized_message_t & message)
	{
	auto [type, client_id, sequence_number] = get_msg_event_type(message);

	// Ignore response message
	if (type == service_msgs::msg::ServiceEventInfo::RESPONSE_SENT \|\|
	type == service_msgs::msg::ServiceEventInfo::RESPONSE_RECEIVED)
	{
	return false;
	}

	bool ret = false;

	// For each Client, decide which request data to use based on the first message related to
	// the request that is obtained from the record data.
	// e.g.

	auto iter = request_info_.find(client_id);
	if (type == service_msgs::msg::ServiceEventInfo::REQUEST_RECEIVED) {
	if (!service_set_introspection_content_) {
	if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {
	service_set_introspection_content_ = true;
	}
	}

	if (iter != request_info_.end()) {
	switch (iter->second) {
	case request_info_from::CLIENT:
	{
	// Already decide using request data from client.
	break;
	}
	case request_info_from::NO_CONTENT:
	{
	if (service_set_introspection_content_) {
	// introspection type is changed from metadata to metadata + contents
	request_info_[client_id] = request_info_from::SERVICE;
	ret = true;
	} else {
	RCUTILS_LOG_WARN_ONCE_NAMED(
	ROSBAG2_TRANSPORT_PACKAGE_NAME,
	"The configuration of introspection for '%s' is metadata on service side !",
	service_name_.c_str());
	}
	break;
	}
	default: // request_info_from::SERVICE:
	{
	// Already decide using request data from service.
	ret = true;
	}
	}
	} else {
	if (service_set_introspection_content_) {
	request_info_[client_id] = request_info_from::SERVICE;
	ret = true;
	} else {
	request_info_[client_id] = request_info_from::NO_CONTENT; // Only have metadata
	}
	}

	return ret;
	}

	// type is service_msgs::msg::ServiceEventInfo::REQUEST_SENT
	if (iter != request_info_.end()) {
	switch (iter->second) {
	case request_info_from::CLIENT:
	{
	// Already decide using request data from client.
	ret = true;
	break;
	}
	case request_info_from::NO_CONTENT:
	{
	if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {
	// introspection type is changed from metadata to metadata + contents
	request_info_[client_id] = request_info_from::CLIENT;
	ret = true;
	} else {
	RCUTILS_LOG_WARN_ONCE_NAMED(
	ROSBAG2_TRANSPORT_PACKAGE_NAME,
	"The configuration of introspection for '%s' client [ID: %s]` is metadata !",
	rosbag2_cpp::client_id_to_string(client_id).c_str(),
	service_name_.c_str());
	}
	break;
	}
	default: // request_info_from::SERVICE:
	{
	// Already decide using request data from service.
	ret = false;
	}
	}
	} else {
	if (rosbag2_cpp::service_event_include_metadata_and_contents(message.buffer_length)) {
	request_info_[client_id] = request_info_from::CLIENT;
	ret = true;
	} else {
	request_info_[client_id] = request_info_from::NO_CONTENT;
	}
	}

	return ret;
	}

Fixes for services playback per review 2 #6

Fixes for services playback per review 2 #6

Conversation

MichaelOrlov commented Mar 31, 2024

Made the following changes per review:

MichaelOrlov commented Mar 31, 2024

fujitatomoya commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Apr 1, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Apr 12, 2024 • edited Loading

Barry-Xu-2018 commented Apr 12, 2024 •

edited

Loading