Fix a bug on adding unnecessary done callback of future while repeatedly calling spin_until_future_complete

[Barry-Xu-2018]

Address a bug while investigating #1371

If write action client code like below,

            while rclpy.ok:
                future = action_client.send_goal(xxx)
                rclpy.spin_until_future_complete(action_client, future)

In spin_until_future_complete(), a callback is added to the done callback of future. And next spin_once_until_future_complete() will be called continuously.

rclpy/rclpy/rclpy/executors.py

Lines 314 to 332 in a09a031

	def spin_until_future_complete(
	self,
	future: Future,
	timeout_sec: Optional[float] = None
	) -> None:
	"""Execute callbacks until a given future is done or a timeout occurs."""
	# Make sure the future wakes this executor when it is done
	future.add_done_callback(lambda x: self.wake())
	if timeout_sec is None or timeout_sec < 0:
	while self._context.ok() and not future.done() and not self._is_shutdown:
	self.spin_once_until_future_complete(future, timeout_sec)
	else:
	start = time.monotonic()
	end = start + timeout_sec
	timeout_left = TimeoutObject(timeout_sec)
	while self._context.ok() and not future.done() and not self._is_shutdown:
	self.spin_once_until_future_complete(future, timeout_left)

Each time spin_once_until_future_complete() is called, the same callback is added to the future's done callback.

rclpy/rclpy/rclpy/executors.py

Lines 855 to 861 in a09a031

	def spin_once_until_future_complete(
	self,
	future: Future,
	timeout_sec: Optional[Union[float, TimeoutObject]] = None
	) -> None:
	future.add_done_callback(lambda x: self.wake())
	self._spin_once_impl(timeout_sec, future.done)

This way, the future's done callbacks keep accumulating.

When a future is completed (calls set_result()), _schedule_or_invoke_done_callbacks() is called.
In this function, it creates a Task for each done callback, which leads to many unnecessary Tasks being created and consuming more and more memory.

rclpy/rclpy/rclpy/task.py

Lines 150 to 153 in a09a031

	if executor is not None:
	# Have the executor take care of the callbacks
	for callback in callbacks:
	executor.create_task(callback, self)

So this PR is to remove the code that adds unnecessary done callbacks.

[fujitatomoya]

lgtm with green CI.

@Barry-Xu-2018 btw could you check if this PR fixes the memory leak issue #1371

[Barry-Xu-2018]

@fujitatomoya

In my environment, the action client memory still increases a bit, but it's much less than before.

[Barry-Xu-2018]

Only update commit description in d495a94

[Barry-Xu-2018]

CI:

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@clalancette @sloretz @ahcorde could either of you review this?

[bmartin427]

Can this be backported? I just tripped over it in Jazzy.

[fujitatomoya]

@Mergifyio backport jazzy

[mergify]

backport jazzy

✅ Backports have been created

• #1530 Fix a bug on adding unnecessary done callback of future while repeatedly calling spin_until_future_complete (backport #1374) has been created for branch jazzy but encountered conflicts