Implement generic client

[Barry-Xu-2018]

This PR depended on #2209.

This interface is added for playing service in rosbag2.

ros2/rosbag2#1481 depended on this PR.

[fujitatomoya]

ros2/rosbag2#1480, and ros2/rosbag2#1481 depended on this PR.

@Barry-Xu-2018 this one too, we should close this?

[fujitatomoya]

Ah sorry, this one is required to play service, right? is this still in draft or i can start the review?

[Barry-Xu-2018]

Ah sorry, this one is required to play service, right? is this still in draft or i can start the review?

Yes.
This PR was based on #2209 and 2209 isn't merged.
So current changed files of this PR include changed file for 2209.
After 2209 is merged, the changes made in this PR can be displayed more clearly.
So I set draft for this PR.

[Barry-Xu-2018]

Now #2209 has been merged.
So do rebase and ready to review.

[Barry-Xu-2018]

Build failure is because used rcl codes are old.

20:43:25 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/logger.cpp:84:9: error: ‘RCL_RET_NOT_FOUND’ was not declared in this scope; did you mean ‘RCUTILS_RET_NOT_FOUND’?
20:43:25    84 |     if (RCL_RET_NOT_FOUND == rcl_ret) {
20:43:25       |         ^~~~~~~~~~~~~~~~~
20:43:25       |         RCUTILS_RET_NOT_FOUND

[fujitatomoya]

overall lgtm, several comments.

[fujitatomoya]

@iuhilnehc-ynos can you do another review on this?

[iuhilnehc-ynos]

some minor comments.

[Barry-Xu-2018]

@iuhilnehc-ynos

Thank you for your comments. I will handle them.

[fujitatomoya]

lgtm, one comment needs to be resolved.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[Barry-Xu-2018]

lgtm, one comment needs to be resolved.

Sorry, I missed it. I will fix it.

[Barry-Xu-2018]

After checking log, the CI failure for Linux is related to CI system.
About the CI failure for Window, I will investigate it.

[Barry-Xu-2018]

@fujitatomoya Please help re-run the CI.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[Barry-Xu-2018]

For Linux-aarch64, the failure case 'ros2topic.ros2topic.test.test_cli.test_cli' is unrelated to this patch.

[wjwwood]

I had a few requested changes and questions.

One of the bigger issues I have with the implementation (as opposed to the concept) is that we're copy-pasting a lot of code from the client class for this new generic class, and especially for the tests it's a lot. I think the goal for the tests should be to (as often as possible) have the same tests run over both the normal and generic client classes and where it doesn't make sense it should be easy to see which tests don't apply to the generic client and why.

[wjwwood]

nitpick: if you need to multi-line the function prototypes, I think it's cleaner to put the return code on its own line first, then the arguments, and if you do that for one function, it's helpful for readability (again in my opinion) to put them all on their own line even for functions that are short. This squarely falls into developer discretion, but I wanted to point it out. To be clear, I'm suggesting:

Suggested change

	RCLCPP_PUBLIC
	void handle_response(
	std::shared_ptr<rmw_request_id_t> request_header,
	std::shared_ptr<void> response) override;
	RCLCPP_PUBLIC
	void
	handle_response(
	std::shared_ptr<rmw_request_id_t> request_header,
	std::shared_ptr<void> response) override;

[Barry-Xu-2018]

I will update other declarations to use this coding style.

[wjwwood]

Copy and pasting large chunks of logic like this one (pulled from

rclcpp/rclcpp/include/rclcpp/client.hpp

Lines 761 to 787 in 126d517

	/// Clean all pending requests older than a time_point.
	/**
	* \param[in] time_point Requests that were sent before this point are going to be removed.
	* \param[inout] pruned_requests Removed requests id will be pushed to the vector
	* if a pointer is provided.
	* \return number of pending requests that were removed.
	*/
	template<typename AllocatorT = std::allocator<int64_t>>
	size_t
	prune_requests_older_than(
	std::chrono::time_point<std::chrono::system_clock> time_point,
	std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
	{
	std::lock_guard guard(pending_requests_mutex_);
	auto old_size = pending_requests_.size();
	for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
	if (it->second.first < time_point) {
	if (pruned_requests) {
	pruned_requests->push_back(it->first);
	}
	it = pending_requests_.erase(it);
	} else {
	++it;
	}
	}
	return old_size - pending_requests_.size();
	}

) is really bad imo. If we discover and fix a bug in one we may fail to update the other. It would be better to factor these into common functions, so that it is more obvious that this logic is used in two places.

[Barry-Xu-2018]

@wjwwood

"Currently, pending_requests_ is defined in GenericClient class and Client class. The operations on pending_requests_ involve duplicated code.
There are two ways to avoid duplicated codes:

1. Move pending_requests_ and the corresponding mutex lock into the client base class. Since pending_requests_ contains different contents, the BaseClient class must be changed to a template class. This change would be quite significant.

2. Use macros to include the same code and place the macros in client.hpp."

I am inclined towards 2.
I would like to hear your opinion.

[wjwwood]

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

[Barry-Xu-2018]

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

Yes. Using function is okay.
pending_requests_ must be passed, and the type of pending_requests_ is different for Client and GenericClient. So the function have to be a template function.

[wjwwood]

Similar comment here, let's not copy-paste tests (this is from

rclcpp/rclcpp/test/rclcpp/test_client.cpp

Lines 413 to 503 in 126d517

	/*
	Testing on_new_response callbacks.
	*/
	TEST_F(TestClient, on_new_response_callback) {
	auto client_node = std::make_shared<rclcpp::Node>("client_node", "ns");
	auto server_node = std::make_shared<rclcpp::Node>("server_node", "ns");
	rclcpp::ServicesQoS client_qos;
	client_qos.keep_last(3);
	auto client = client_node->create_client<test_msgs::srv::Empty>("test_service", client_qos);
	std::atomic<size_t> server_requests_count {0};
	auto server_callback = [&server_requests_count](
	const test_msgs::srv::Empty::Request::SharedPtr,
	test_msgs::srv::Empty::Response::SharedPtr) {server_requests_count++;};
	auto server = server_node->create_service<test_msgs::srv::Empty>(
	"test_service", server_callback, client_qos);
	auto request = std::make_shared<test_msgs::srv::Empty::Request>();
	std::atomic<size_t> c1 {0};
	auto increase_c1_cb = [&c1](size_t count_msgs) {c1 += count_msgs;};
	client->set_on_new_response_callback(increase_c1_cb);
	client->async_send_request(request);
	auto start = std::chrono::steady_clock::now();
	while (server_requests_count == 0 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}
	ASSERT_EQ(server_requests_count, 1u);
	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);
	EXPECT_EQ(c1.load(), 1u);
	std::atomic<size_t> c2 {0};
	auto increase_c2_cb = [&c2](size_t count_msgs) {c2 += count_msgs;};
	client->set_on_new_response_callback(increase_c2_cb);
	client->async_send_request(request);
	start = std::chrono::steady_clock::now();
	while (server_requests_count == 1 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}
	ASSERT_EQ(server_requests_count, 2u);
	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c1 == 0 && std::chrono::steady_clock::now() - start < 10s);
	EXPECT_EQ(c1.load(), 1u);
	EXPECT_EQ(c2.load(), 1u);
	client->clear_on_new_response_callback();
	client->async_send_request(request);
	client->async_send_request(request);
	client->async_send_request(request);
	start = std::chrono::steady_clock::now();
	while (server_requests_count < 5 &&
	(std::chrono::steady_clock::now() - start) < 10s)
	{
	rclcpp::spin_some(server_node);
	}
	ASSERT_EQ(server_requests_count, 5u);
	std::atomic<size_t> c3 {0};
	auto increase_c3_cb = [&c3](size_t count_msgs) {c3 += count_msgs;};
	client->set_on_new_response_callback(increase_c3_cb);
	start = std::chrono::steady_clock::now();
	do {
	std::this_thread::sleep_for(100ms);
	} while (c3 < 3 && std::chrono::steady_clock::now() - start < 10s);
	EXPECT_EQ(c1.load(), 1u);
	EXPECT_EQ(c2.load(), 1u);
	EXPECT_EQ(c3.load(), 3u);
	std::function<void(size_t)> invalid_cb = nullptr;
	EXPECT_THROW(client->set_on_new_response_callback(invalid_cb), std::invalid_argument);
	}

and it's not just this test, it's most of this file) that are hugely complex, instead please refactor them to have the same code run over Generic and regular Clients.

[Barry-Xu-2018]

@wjwwood
Thank you for your advice.

One of the bigger issues I have with the implementation (as opposed to the concept) is that we're copy-pasting a lot of code from the client class for this new generic class, and especially for the tests it's a lot. I think the goal for the tests should be to (as often as possible) have the same tests run over both the normal and generic client classes and where it doesn't make sense it should be easy to see which tests don't apply to the generic client and why.

Yes. I will optimize test codes.

[wjwwood]

Thanks, you can have a look at some of the executor tests for examples of tests that run over several different classes by parameterization of the test.

[wjwwood]

rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp

Lines 121 to 146 in 6f6b5f2

	// TYPED_TEST_SUITE is deprecated as of gtest 1.9, use TYPED_TEST_SUITE when gtest dependency
	// is updated.
	TYPED_TEST_SUITE(TestExecutors, ExecutorTypes, ExecutorTypeNames);
	// StaticSingleThreadedExecutor is not included in these tests for now, due to:
	// https://github.com/ros2/rclcpp/issues/1219
	using StandardExecutors =
	::testing::Types<
	rclcpp::executors::SingleThreadedExecutor,
	rclcpp::executors::MultiThreadedExecutor,
	rclcpp::experimental::executors::EventsExecutor>;
	TYPED_TEST_SUITE(TestExecutorsStable, StandardExecutors, ExecutorTypeNames);
	// Make sure that executors detach from nodes when destructing
	TYPED_TEST(TestExecutors, detachOnDestruction)
	{
	using ExecutorType = TypeParam;
	{
	ExecutorType executor;
	executor.add_node(this->node);
	}
	{
	ExecutorType executor;
	EXPECT_NO_THROW(executor.add_node(this->node));
	}
	}

[Barry-Xu-2018]

@wjwwood

I submitted changes 69145c4.

1. Move common tests for Client and GenericClient to test_client_common.cpp.
2. Add new interface create_generic_client with NodeT parameter. Implement generic client #2358 (comment)
3. Use function for common function Implement generic client #2358 (comment)

[fujitatomoya]

me and @iuhilnehc-ynos 's comments are resolved. i will start the Full CI.

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@wjwwood

I submitted changes 69145c4.

1. Move common tests for Client and GenericClient to test_client_common.cpp.
2. Add new interface create_generic_client with NodeT parameter. Implement generic client #2358 (comment)
3. Use function for common function Implement generic client #2358 (comment)

@wjwwood can you review your comments?

[Barry-Xu-2018]

• Linux
• Linux-aarch64
• Windows

I checked failed cases.
The reason for the test failure is unrelated to this PR.

[wjwwood]

lgtm with passing CI

Thank you for iterating and removing a lot of the duplicate code! It looks much better now.