Feature: add executor.create_future()

[nadavelkabets]

Part of #1399 and #1469.
To allow an executor to reschedule a task when a future stops blocking, it is crucial that the future is attached to the same executor.
Currently, users are able to initialize a Future instance without supplying the executor parameter.
This behavior is necessary to allow calling client.call_async() before attaching the node to an executor (the future is attached to the running executor when processing the service response).

In asyncio, loop.create_future() was added to discourage users from initializing the Future class directly.
I suggest we follow the same path.

[nadavelkabets]

@sloretz @fujitatomoya @mjcarroll @wjwwood @ahcorde
I'd appreciate your feedback.

[fujitatomoya]

i think this makes sense, that makes the explicit association to the executor, the future is tied to the executor that created it. and it also aligns with create_task method in API consistency.

i would like to have another approval before merge.

[bjsowa]

LGTM. I would also change the other tests in test_executor to use the new method for creating futures.

[bjsowa]

Also, shouldn't the Client and ActionClient now use this method when creating a future?

[nadavelkabets]

Also, shouldn't the Client and ActionClient now use this method when creating a future?

@bjsowa @fujitatomoya
Client and ActionClient initialize the Future object without an executor by design.
Currently, users are able to execute client.call_async() before initializing an executor.
To allow this behavior, Future is initialized without an executor, and the executor is set when processing the service response.

rclpy/rclpy/rclpy/executors.py

Line 516 in 0fbd01a

future._set_executor(self)

The same case applies to ActionClient (or any other Waitable)

rclpy/rclpy/rclpy/executors.py

Line 566 in 0fbd01a

future._set_executor(self)

LGTM. I would also change the other tests in test_executor to use the new method for creating futures.

Done

[bjsowa]

Client and ActionClient initialize the Future object without an executor by design.
Currently, users are able to execute client.call_async() before initializing an executor.
To allow this behavior, Future is initialized without an executor, and the executor is set when processing the service response.

Oh, I haven't realized that. That makes sense.

[fujitatomoya]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[fujitatomoya]

Pulls: #1495
Gist: https://gist.githubusercontent.com/fujitatomoya/040a246466d852d7f1e2f76dcebe805f/raw/02653b41ea5877bd104f5c846004187e68069c89/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16894

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

[nadavelkabets]

@fujitatomoya could you run CI again? I believe Linux failed for unrelated reasons.

[fujitatomoya]

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

[nadavelkabets]

@fujitatomoya can we proceed to merge this PR?

[fujitatomoya]

https://ci.ros2.org/job/ci_linux-rhel/4284/ are known issues and unrelated.

[nadavelkabets]

@fujitatomoya thank you for helping to merge this PR. I really appreciate your support🙏
Do you think we could backport this feature to jazzy?

[fujitatomoya]

i think that it is straight-forward for kilted, but maybe there can be conflict for jazzy. but i think that it is backportable.

[fujitatomoya]

@Mergifyio backport kilted jazzy

[mergify]

backport kilted jazzy

✅ Backports have been created

Details

• #1499 Feature: add executor.create_future() (backport #1495) has been created for branch kilted
• #1500 Feature: add executor.create_future() (backport #1495) has been created for branch jazzy but encountered conflicts