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.

Sign up for GitHub

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

Jump to bottom

Fix Memory Leak in Actions Using EventsExecutor #2661

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Fix Memory Leak in Actions Using EventsExecutor #2661

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d839fd
Fix Actions memory leak: Add timer to cleanup
Nov 4, 2024
f4408aa
Use a single timer to expire goals
Nov 5, 2024
bfebe0a
Add missing includes
Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rclcpp_action/include/rclcpp_action/create_server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_clock_interface.hpp"
#include "rclcpp/node_interfaces/node_logging_interface.hpp"
#include "rclcpp/node_interfaces/node_timers_interface.hpp"
#include "rclcpp/node_interfaces/node_waitables_interface.hpp"

#include "rclcpp_action/server.hpp"
Expand All @@ -42,6 +43,7 @@ namespace rclcpp_action
* \param[in] node_base_interface The node base interface of the corresponding node.
* \param[in] node_clock_interface The node clock interface of the corresponding node.
* \param[in] node_logging_interface The node logging interface of the corresponding node.
* \param[in] node_timers_interface The node timers interface of the corresponding node.
* \param[in] node_waitables_interface The node waitables interface of the corresponding node.
* \param[in] name The action name.
* \param[in] handle_goal A callback that decides if a goal should be accepted or rejected.
Expand All @@ -59,6 +61,7 @@ create_server(
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock_interface,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface,
rclcpp::node_interfaces::NodeTimersInterface::SharedPtr node_timers_interface,
rclcpp::node_interfaces::NodeWaitablesInterface::SharedPtr node_waitables_interface,
const std::string & name,
typename Server<ActionT>::GoalCallback handle_goal,
Expand Down Expand Up @@ -100,6 +103,7 @@ create_server(
node_base_interface,
node_clock_interface,
node_logging_interface,
node_timers_interface,
name,
options,
handle_goal,
Expand Down Expand Up @@ -142,6 +146,7 @@ create_server(
node->get_node_base_interface(),
node->get_node_clock_interface(),
node->get_node_logging_interface(),
node->get_node_timers_interface(),
node->get_node_waitables_interface(),
name,
handle_goal,
Expand Down
22 changes: 22 additions & 0 deletions rclcpp_action/include/rclcpp_action/server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_clock_interface.hpp"
#include "rclcpp/node_interfaces/node_logging_interface.hpp"
#include "rclcpp/node_interfaces/node_timers_interface.hpp"
#include "rclcpp/waitable.hpp"

#include "rclcpp_action/visibility_control.hpp"
Expand Down Expand Up @@ -173,6 +174,17 @@ class ServerBase : public rclcpp::Waitable
void
set_on_ready_callback(std::function<void(size_t, int)> callback) override;

/// \internal
/// Set up a one-shot timer to trigger when a goal's expiration time is reached
/**
* Initializes a timer to trigger a callback when a goal expires,
* enabling cleanup of expired goals. The timer is removed after the callback
* is called
*/
RCLCPP_ACTION_PUBLIC
void
setup_expire_goal_timer();

/// Unset the callback to be called whenever the waitable becomes ready.
RCLCPP_ACTION_PUBLIC
void
Expand All @@ -187,6 +199,7 @@ class ServerBase : public rclcpp::Waitable
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging,
rclcpp::node_interfaces::NodeTimersInterface::SharedPtr node_timers,
const std::string & name,
const rosidl_action_type_support_t * type_support,
const rcl_action_server_options_t & options);
Expand Down Expand Up @@ -330,6 +343,13 @@ class ServerBase : public rclcpp::Waitable
// Storage for std::function callbacks to keep them in scope
std::unordered_map<EntityType, std::function<void(size_t)>> entity_type_to_on_ready_callback_;

// Store elements required to create goal expiration timers
std::mutex expire_goal_timers_mutex_;
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_;
rclcpp::node_interfaces::NodeTimersInterface::SharedPtr node_timers_;
std::vector<std::pair<unsigned int, rclcpp::TimerBase::SharedPtr>> expire_goal_timers_;
mauropasse marked this conversation as resolved.
Show resolved Hide resolved
rcl_duration_value_t goal_expire_timeout_;

/// Set a callback to be called when the specified entity is ready
RCLCPP_ACTION_PUBLIC
void
Expand Down Expand Up @@ -396,6 +416,7 @@ class Server : public ServerBase, public std::enable_shared_from_this<Server<Act
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging,
rclcpp::node_interfaces::NodeTimersInterface::SharedPtr node_timers,
const std::string & name,
const rcl_action_server_options_t & options,
GoalCallback handle_goal,
Expand All @@ -406,6 +427,7 @@ class Server : public ServerBase, public std::enable_shared_from_this<Server<Act
node_base,
node_clock,
node_logging,
node_timers,
name,
rosidl_typesupport_cpp::get_action_type_support_handle<ActionT>(),
options),
Expand Down
41 changes: 40 additions & 1 deletion rclcpp_action/src/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "action_msgs/msg/goal_status_array.hpp"
#include "rclcpp/exceptions.hpp"
#include "rclcpp/create_timer.hpp"
#include "rclcpp_action/server.hpp"

using rclcpp_action::ServerBase;
Expand Down Expand Up @@ -117,12 +118,15 @@ ServerBase::ServerBase(
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging,
rclcpp::node_interfaces::NodeTimersInterface::SharedPtr node_timers,
const std::string & name,
const rosidl_action_type_support_t * type_support,
const rcl_action_server_options_t & options
)
: pimpl_(new ServerBaseImpl(
node_clock->get_clock(), node_logging->get_logger().get_child("rclcpp_action")))
node_clock->get_clock(), node_logging->get_logger().get_child("rclcpp_action"))),
node_base_(node_base), node_timers_(node_timers),
goal_expire_timeout_(options.result_timeout.nanoseconds)
{
auto deleter = [node_base](rcl_action_server_t * ptr)
{
Expand Down Expand Up @@ -645,6 +649,37 @@ ServerBase::execute_check_expired_goals()
}
}

void ServerBase::setup_expire_goal_timer()
{
std::lock_guard<std::mutex> lock(expire_goal_timers_mutex_);
unsigned int timer_id = expire_goal_timers_.empty() ? 1 : expire_goal_timers_.back().first + 1;

auto one_shot_timer_callback = [this, timer_id]() mutable {
execute_check_expired_goals();

std::lock_guard<std::mutex> lock_mutex(expire_goal_timers_mutex_);
auto it = std::find_if(
mauropasse marked this conversation as resolved.
Show resolved Hide resolved
expire_goal_timers_.begin(), expire_goal_timers_.end(),
[timer_id](const std::pair<unsigned int, rclcpp::TimerBase::SharedPtr>& element) {
return element.first == timer_id;
});

if (it != expire_goal_timers_.end()) {
expire_goal_timers_.erase(it);
}
};

auto timer = rclcpp::create_wall_timer(
std::chrono::nanoseconds(goal_expire_timeout_),
mauropasse marked this conversation as resolved.
Show resolved Hide resolved
std::function<void()>(one_shot_timer_callback),
nullptr,
node_base_.get(),
node_timers_.get()
);
mauropasse marked this conversation as resolved.
Show resolved Hide resolved

expire_goal_timers_.emplace_back(timer_id, timer);
}

void
ServerBase::publish_status()
{
Expand Down Expand Up @@ -720,6 +755,10 @@ ServerBase::publish_result(const GoalUUID & uuid, std::shared_ptr<void> result_m
throw std::runtime_error("Asked to publish result for goal that does not exist");
}

if (on_ready_callback_set_) {
setup_expire_goal_timer();
}

{
/**
* NOTE: There is a potential deadlock issue if both unordered_map_mutex_ and
Expand Down