Skip to content

Fix bug on the disorder of calling shutdown callback#2097

Merged
clalancette merged 2 commits intoros2:rollingfrom
Barry-Xu-2018:topic_fix_bug_on_shutdown_callback_order
Feb 6, 2023
Merged

Fix bug on the disorder of calling shutdown callback#2097
clalancette merged 2 commits intoros2:rollingfrom
Barry-Xu-2018:topic_fix_bug_on_shutdown_callback_order

Conversation

@Barry-Xu-2018
Copy link
Copy Markdown
Collaborator

Address #2096

Signed-off-by: Barry Xu barry.xu@sony.com

Signed-off-by: Barry Xu <barry.xu@sony.com>
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One very minor thing pointed out inline. Otherwise looks good with green CI.

Copy link
Copy Markdown
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.

lgtm! @Barry-Xu-2018 thanks for quick response.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@clalancette @alsora thanks for the review, btw can we backport this to humble?

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Feb 2, 2023

CI:

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

@clalancette
Copy link
Copy Markdown
Contributor

@clalancette @alsora thanks for the review, btw can we backport this to humble?

Hm. So unfortunately, this breaks ABI because of the change of type from std::unordered_set to std::vector. So this would not generally be eligible for backporting. We'd have to consider another non-ABI breaking way to backport this, or explicitly make an exception here.

Signed-off-by: Barry Xu <barry.xu@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator

  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@clalancette if the backport is hard requirement, we would probably consider other approach. I believe that breaking ABI/API policy would be much bigger problem.

@Barry-Xu-2018
Copy link
Copy Markdown
Collaborator Author

@clalancette @fujitatomoya

In order to keep ABI compatible, we cannot add new variable (except static variable) into class Context.
Maybe use global variable as one solution, but it will make code ugly.

struct ShutdownCallback {
  std::vector<weak_ptr> pre_callback;
  std::vector<weak_ptr> on_callback;
};

static std:map<Context*, ShutdownCallback> g_shutdownCallback;
static mutex s_shutdownCallback_mutex;

Change code on adding/removing/call shutdown callback to use this global variable.

Now I not find better approach

@clalancette
Copy link
Copy Markdown
Contributor

I'm going to go ahead and merge this one in, since it is an obvious improvement for Rolling. We can consider fixing Humble (either the documentation or with an ABI-compatible fix) separately.

@clalancette clalancette merged commit 1fd5a96 into ros2:rolling Feb 6, 2023
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
* Fix bug on the disorder of calling shutdown callback

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
* Fix bug on the disorder of calling shutdown callback

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
* Fix bug on the disorder of calling shutdown callback

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
* Fix bug on the disorder of calling shutdown callback

Signed-off-by: Barry Xu <barry.xu@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants