Skip to content

Feature: add executor.create_future() #1495

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
Merged
merged 2 commits into from
Sep 11, 2025
Merged

Feature: add executor.create_future() #1495

merged 2 commits into from
Sep 11, 2025

Conversation

@nadavelkabets
Copy link
Contributor

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
Copy link
Contributor Author

nadavelkabets commented Aug 30, 2025 •
edited
Loading

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

fujitatomoya
fujitatomoya approved these changes Sep 1, 2025
View reviewed changes
Copy link
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.

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
bjsowa approved these changes Sep 2, 2025
View reviewed changes
Copy link

@bjsowa bjsowa left a comment

Choose a reason for hiding this comment

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

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

@bjsowa
Copy link

bjsowa commented Sep 2, 2025

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

@nadavelkabets
Copy link
Contributor Author

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
bjsowa approved these changes Sep 6, 2025
View reviewed changes
Copy link

@bjsowa bjsowa left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2025

rebase

✅ Branch has been successfully rebased

@fujitatomoya
Copy link
Collaborator

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 Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@nadavelkabets
Copy link
Contributor Author

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

@fujitatomoya
Copy link
Collaborator

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

@nadavelkabets
Copy link
Contributor Author

@fujitatomoya can we proceed to merge this PR?

@fujitatomoya
Copy link
Collaborator

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

@fujitatomoya fujitatomoya merged commit bcdd663 into ros2:rolling Sep 11, 2025
3 checks passed
@nadavelkabets
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

@Mergifyio backport kilted jazzy

@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2025 •
edited
Loading

backport kilted jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 14, 2025
@nadavelkabets @mergify
Feature: add executor.create_future() (#1495)
5e99f55 
* feature: add create_future and test

Signed-off-by: Nadav Elkabets <[email protected]>

* Use create_future in all executor tests

Signed-off-by: Nadav Elkabets <[email protected]>

---------

Signed-off-by: Nadav Elkabets <[email protected]>
(cherry picked from commit bcdd663)
mergify bot pushed a commit that referenced this pull request Sep 14, 2025
@nadavelkabets @mergify
Feature: add executor.create_future() (#1495)
8a36b9f 
* feature: add create_future and test

Signed-off-by: Nadav Elkabets <[email protected]>

* Use create_future in all executor tests

Signed-off-by: Nadav Elkabets <[email protected]>

---------

Signed-off-by: Nadav Elkabets <[email protected]>
(cherry picked from commit bcdd663)

# Conflicts:
#	rclpy/src/rclpy/events_executor/events_executor.cpp
#	rclpy/src/rclpy/events_executor/events_executor.hpp
#	rclpy/test/test_executor.py
This was referenced Sep 14, 2025
Merged
Merged
fujitatomoya pushed a commit that referenced this pull request Sep 15, 2025
@mergify @nadavelkabets
Feature: add executor.create_future() (#1495) (#1499)
572a297 
* feature: add create_future and test



* Use create_future in all executor tests



---------


(cherry picked from commit bcdd663)

Signed-off-by: Nadav Elkabets <[email protected]>
Co-authored-by: Nadav Elkabets <[email protected]>
fujitatomoya added a commit that referenced this pull request Sep 16, 2025
@mergify @nadavelkabets @fujitatomoya
Feature: add executor.create_future() (backport #1495) (#1500)
ec40fc6 
* Feature: add executor.create_future() (#1495)

* feature: add create_future and test

Signed-off-by: Nadav Elkabets <[email protected]>

* Use create_future in all executor tests

Signed-off-by: Nadav Elkabets <[email protected]>

---------

Signed-off-by: Nadav Elkabets <[email protected]>
(cherry picked from commit bcdd663)

# Conflicts:
#	rclpy/src/rclpy/events_executor/events_executor.cpp
#	rclpy/src/rclpy/events_executor/events_executor.hpp
#	rclpy/test/test_executor.py

* resolve conflicts.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Co-authored-by: Nadav Elkabets <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

+1 more reviewer

@bjsowa bjsowa bjsowa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nadavelkabets @bjsowa @fujitatomoya