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

Fix #1526. #1527

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Fix #1526. #1527

merged 2 commits into from
Jan 26, 2021

Conversation

y-okumura-isp
Copy link
Contributor

Fixes #1526.

As action_server_ depends on clock_, we declare clock_ first.
Then the order of deletion becomes action_server_, clock_.

There are still errors in the following test, but we have solved errors for test_server.

  • test_client
  • test_server_goal_handle

As action_server_ depends on clock_, we declare clock_ first.
Then the order of deletion becomes action_server_, clock_.

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that this makes sense,

auto deleter = [node_base](rcl_action_server_t * ptr)
{
if (nullptr != ptr) {
rcl_node_t * rcl_node = node_base->get_rcl_node_handle();
rcl_ret_t ret = rcl_action_server_fini(ptr, rcl_node);
if (RCL_RET_OK != ret) {
RCLCPP_DEBUG(
rclcpp::get_logger("rclcpp_action"),
"failed to fini rcl_action_server_t in deleter");
}
delete ptr;
}
};

ServerBaseImpl::action_server_ requires ServerBaseImpl::clock_ during deletion.

@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

@ivanpauno all green, could you take a look just in case?

rclcpp_action/src/server.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno added the bug Something isn't working label Jan 25, 2021
Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
@fujitatomoya
Copy link
Collaborator

Only comments changed since last CI, i will go ahead to merge this.

@fujitatomoya fujitatomoya merged commit 4da9a0c into ros2:master Jan 26, 2021
Karsten1987 pushed a commit that referenced this pull request Mar 2, 2021
* Fix #1526.

As action_server_ depends on clock_, we declare clock_ first.
Then the order of deletion becomes action_server_, clock_.

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>

* Add comments about declaration order (#1526)

Signed-off-by: y-okumura-isp <y-okumura@isp.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASAN error in rclcpp_action test_server
3 participants