Do not finalize goal handles in expire function

Instead, leave it up to the caller to finalize goal handles.
Renamed the function to rcl_action_expire_goals.

rcl_action/include/rcl_action/action_server.h

@@ -559,37 +559,48 @@ rcl_action_send_result_response(
   const rcl_action_server_t * action_server,
   void * ros_result_response);
 
-/// Clear all expired goals associated with an action server.
+/// Expires goals associated with an action server.
 /**
- * A goal is 'expired' if it has been in a terminal state (has a result) for more
+ * A goal is 'expired' if it has been in a terminal state (has a result) for longer
  * than some duration.
  * The timeout duration is set as part of the action server options.
  *
  * If a negative timeout value if provided, then goal results never expire (kept forever).
  * If a timeout of zero is set, then goal results are discarded immediately (ie. goal
  * results are discarded whenever this function is called).
  *
+ * Expired goals are removed from the internal array of goal handles.
+ * rcl_action_server_goal_exists() will return false for any goals that have expired.
+ *
+ * \attention If one or more goals are expired then a previously returned goal handle
+ * array from rcl_action_server_get_goal_handles() becomes invalid.
+ *
+ * `num_expired` is an optional argument. If it is not `NULL`, then it is set to the
+ * number of goals that were expired.
+ *
  * <hr>
  * Attribute          | Adherence
  * ------------------ | -------------
- * Allocates Memory   | No
+ * Allocates Memory   | Maybe[1]
  * Thread-Safe        | No
  * Uses Atomics       | No
  * Lock-Free          | Yes
+ * <i>[1] if one or more goals expires, then the internal goal handle array may be
+ * resized or deallocated</i>
  *
  * \param[in] action_server handle to the action server from which expired goals
  *   will be cleared.
- * \param[out] num_expired the number of expired goals cleared. If `NULL` then the
- *   number is not set.
+ * \param[out] num_expired the number of expired goals, or set to `NULL` if unused
  * \return `RCL_RET_OK` if the response was sent successfully, or
  * \return `RCL_RET_ACTION_SERVER_INVALID` if the action server is invalid, or
- * \return `RCL_RET_INVALID_ARGUMENT` if num_expired is null, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
+ * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
  * \return `RCL_RET_ERROR` if an unspecified error occurs.
  */
 RCL_ACTION_PUBLIC
 RCL_WARN_UNUSED
 rcl_ret_t
-rcl_action_clear_expired_goals(
+rcl_action_expire_goals(
   const rcl_action_server_t * action_server,
   size_t * num_expired);
 
@@ -764,11 +775,10 @@ rcl_action_server_get_options(const rcl_action_server_t * action_server);
 /**
  * A pointer to the internally held array of pointers to goal handle structs is returned
  * along with the number of items in the array.
- * Goals that have terminated, successfully responded to a client with a
- * result, and have expired (timed out) are not present in the array.
  *
- * The returned handle is made invalid if the action server is finalized or if
- * rcl_shutdown() is called.
+ * The returned handle is made invalid if the action server is finalized, if
+ * rcl_shutdown() is called, or if rcl_action_expire_goals() is called and one or more
+ * goals are expired.
  * The returned handle is not guaranteed to be valid for the life time of the
  * action server as it may be finalized and recreated itself.
  * Therefore, it is recommended to get the handle from the action server using

rcl_action/src/rcl_action/action_server.c

@@ -485,14 +485,13 @@ rcl_action_send_result_response(
 }
 
 rcl_ret_t
-rcl_action_clear_expired_goals(
+rcl_action_expire_goals(
   const rcl_action_server_t * action_server,
   size_t * num_expired)
 {
   if (!rcl_action_server_is_valid(action_server)) {
     return RCL_RET_ACTION_SERVER_INVALID;
   }
-  RCL_CHECK_ARGUMENT_FOR_NULL(num_expired, RCL_RET_INVALID_ARGUMENT);
 
   // Get current time (nanosec)
   int64_t current_time;
@@ -504,7 +503,7 @@ rcl_action_clear_expired_goals(
   // Used for shrinking goal handle array
   rcl_allocator_t allocator = action_server->impl->options.allocator;
 
-  *num_expired = 0u;
+  size_t num_goals_expired = 0u;
   rcl_ret_t ret_final = RCL_RET_OK;
   const int64_t timeout = (int64_t)action_server->impl->options.result_timeout.nanoseconds;
   rcl_action_goal_handle_t * goal_handle;
@@ -526,20 +525,15 @@ rcl_action_clear_expired_goals(
     assert(current_time > goal_time);
     if ((current_time - goal_time) > timeout) {
       // Stop tracking goal handle
-      ret = rcl_action_goal_handle_fini(goal_handle);
-      if (RCL_RET_OK != ret) {
-        ret_final = RCL_RET_ERROR;
-      }
       // Fill in any gaps left in the array with pointers from the end
       action_server->impl->goal_handles[i] = action_server->impl->goal_handles[num_goal_handles];
       action_server->impl->goal_handles[num_goal_handles--] = NULL;
-
-      ++(*num_expired);
+      ++num_goals_expired;
     }
   }
 
   // Shrink goal handle array if some goals expired
-  if (*num_expired > 0u) {
+  if (num_goals_expired > 0u) {
     if (0 == num_goal_handles) {
       allocator.deallocate(action_server->impl->goal_handles, allocator.state);
     } else {
@@ -554,6 +548,11 @@ rcl_action_clear_expired_goals(
       }
     }
   }
+
+  // If argument is not null, then set it
+  if (NULL != num_expired) {
+    (*num_expired) = num_goals_expired;
+  }
   return ret_final;
 }
 

rcl_action/test/rcl_action/test_action_server.cpp

@@ -261,26 +261,25 @@ TEST_F(TestActionServer, test_action_clear_expired_goals)
 {
   size_t num_expired = 1u;
   // Clear expired goals with null action server
-  rcl_ret_t ret = rcl_action_clear_expired_goals(nullptr, &num_expired);
+  rcl_ret_t ret = rcl_action_expire_goals(nullptr, &num_expired);
   EXPECT_EQ(ret, RCL_RET_ACTION_SERVER_INVALID) << rcl_get_error_string().str;
   rcl_reset_error();
 
-  // Clear expired goals with null size argument
-  ret = rcl_action_clear_expired_goals(&this->action_server, nullptr);
-  EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT) << rcl_get_error_string().str;
-  rcl_reset_error();
-
   // Clear with invalid action server
   rcl_action_server_t invalid_action_server = rcl_action_get_zero_initialized_server();
-  ret = rcl_action_clear_expired_goals(&invalid_action_server, &num_expired);
+  ret = rcl_action_expire_goals(&invalid_action_server, &num_expired);
   EXPECT_EQ(ret, RCL_RET_ACTION_SERVER_INVALID) << rcl_get_error_string().str;
   rcl_reset_error();
 
   // Clear with valid arguments
-  ret = rcl_action_clear_expired_goals(&this->action_server, &num_expired);
+  ret = rcl_action_expire_goals(&this->action_server, &num_expired);
   EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
   EXPECT_EQ(num_expired, 0u);
 
+  // Clear with valid arguments (optional num_expired)
+  ret = rcl_action_expire_goals(&this->action_server, nullptr);
+  EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
+
   // TODO(jacobperron): Test with goals that actually expire
 }