Refactor client for multiple requests by sloretz · Pull Request #170 · ros2/rclpy

sloretz · 2018-01-04T22:54:45Z

Opening a PR for feedback. This uses the Future class added in pull request #166 to allow simultaneous requests from the same client (part of #123). It is implemented like option 3 in this comment.

Specifically any ideas about call? This new version should not be called inside a callback because it would deadlock a SingleThreadedExecutor. It also is not very useful outside because an executor would need to be running in another thread for the client response to be taken from rcl.

CI

Linux
Linux-aarch64
macOS
Windows
- Unrelated failing test (timeout): projectroot.test_api_srv_composition_client_first__rmw_fastrtps_cpp
  - Also timed out on nightly_win_deb http://ci.ros2.org/view/nightly/job/nightly_win_deb/764
  - However, it passed in nightly_win_rep http://ci.ros2.org/view/nightly/job/nightly_win_rel/705

mikaelarguedas · 2018-01-31T16:55:54Z

Mentioned it to @sloretz offline but writing it here for book-keeping.
This PR removes API used by various packages so we'll need to update those to use the new API and merge these PRs all together, of the top of my head: demo_nodes_py, examples_rclpy_* and ros2service need corresponding PRs

dirk-thomas · 2018-02-01T00:00:55Z

rclpy/rclpy/client.py

+        """
+        sequence_number = _rclpy.rclpy_send_request(self.client_handle, req)
+        future = Future()
+        self._pending_requests[sequence_number] = future


In order to be defensive should this check / ensure that sequence_number is not already in the dictionary?

Added check in ece3781

dirk-thomas · 2018-02-01T00:02:54Z

rclpy/rclpy/client.py

+        future = Future()
+        self._pending_requests[sequence_number] = future
+
+        def remove_pending_request(future):


The argument isn't being used at the moment. Could this be a method on self instead and look up the to-be-deleted key based on the passed future?

Is bf87341 what you have in mind?

dirk-thomas · 2018-02-01T00:04:54Z

rclpy/rclpy/executors.py

+                # The request was cancelled
+                pass
+            else:
+                future._set_executor(self)


It seems weird that the executor is calling a "private" method on the future?

Ideas? It seems weird to me too. The future needs a reference to an executor to be able to add callbacks.

asyncio solves the problem by expecting futures to be created via executor.create_future() which passes itself as a keyword argument to Future.__init__().

Right now the client doesn't have a reference to the executor, so rclpy can't do the same thing. Instead the client holds onto the future and hopes the user will add the node to an executor.

I don't have a suggestion as to how to change it, and I guess @dirk-thomas doesn't either?

So, I think it's ok to merge like this.

Eliminates Response thread Removes wait_for_future() Adds call_async() that returns a Future instance call() uses call_async() internally

sloretz · 2018-02-07T01:35:26Z

CI including updates to other packages

Linux
Linux-aarch64
macOS
Windows
- rclpy.test.test_timer.test_timer_number_callbacks100hertz Unrelated, though usually it's the 1khz timer test that is flaky not the 100hz test. Intereasting that the 100Hz failed while the 1Khz test passed

wjwwood

lgtm, with some comments

wjwwood · 2018-03-21T21:36:53Z

rclpy/rclpy/executors.py

+                # The request was cancelled
+                pass
+            else:
+                future._set_executor(self)


I don't have a suggestion as to how to change it, and I guess @dirk-thomas doesn't either?

So, I think it's ok to merge like this.

wjwwood · 2018-03-21T21:37:44Z

rclpy/rclpy/__init__.py

        executor.shutdown()
+
+
+def spin_until_future_complete(node, future):


Missing a doc block, but so are many existing functions. It would be nice to add, but not required I guess.

Added in cf17f38

sloretz · 2018-03-23T16:18:58Z

Running CI since it's been a while and cf17f38 was added.

Edit: CI looks good

Linux
Linux-aarch64
macOS
Windows (2 Composition test failures)

rclpy/rclpy/executors.py

…osservice Fix service servers with rosservice

sloretz self-assigned this Jan 4, 2018

sloretz requested a review from mikaelarguedas January 4, 2018 22:54

sloretz added the in progress Actively being worked on (Kanban column) label Jan 4, 2018

sloretz force-pushed the rclpy_client_multiple_requests branch 2 times, most recently from 1182f65 to 6a7d6a9 Compare January 5, 2018 17:15

This was referenced Jan 5, 2018

Clarify that rcl_take_response() populates request_header ros2/rcl#205

Merged

Futures and coroutines #166

Merged

rclpy_take_response checks sequence number #171

Merged

sloretz force-pushed the rclpy_client_multiple_requests branch from 6a7d6a9 to 93bbea4 Compare January 18, 2018 21:06

sloretz changed the base branch from rclpy_future_coroutine to master January 18, 2018 21:07

sloretz changed the title ~~[Seeking feedback] Refactor client for multiple requests~~ Refactor client for multiple requests Jan 19, 2018

sloretz force-pushed the rclpy_client_multiple_requests branch from ec14788 to f60ec40 Compare January 26, 2018 00:54

sloretz mentioned this pull request Jan 26, 2018

Use updated rclpy client api ros2/system_tests#257

Merged

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 26, 2018

dirk-thomas reviewed Feb 1, 2018

View reviewed changes

sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 1, 2018

sloretz mentioned this pull request Feb 2, 2018

Update examples for new client api ros2/examples#203

Merged

sloretz added 7 commits February 6, 2018 16:18

Refactor client for multiple requests

6638141

Eliminates Response thread Removes wait_for_future() Adds call_async() that returns a Future instance call() uses call_async() internally

Remove future when done

9eba492

Add spin_until_future_complete

ec21a07

Add test for concurrent client calls

286e0eb

Make remove_pending_request an instance method

014cf61

Raise if sequence conflicts with a pending request

f21dee7

Fix quotes

1ee7e79

sloretz force-pushed the rclpy_client_multiple_requests branch from 96b94f7 to 1ee7e79 Compare February 7, 2018 00:19

This was referenced Feb 7, 2018

ros2 service Use new client api ros2/ros2cli#77

Merged

Rclpy client multiple requests ros2/demos#228

Merged

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 9, 2018

mikaelarguedas added this to the bouncy milestone Mar 1, 2018

wjwwood approved these changes Mar 21, 2018

View reviewed changes

mikaelarguedas mentioned this pull request Mar 22, 2018

Services feature parity #123

Closed

Docstring for spin_until_future_complete

cf17f38

sloretz merged commit 0c2ee4c into master Mar 27, 2018

sloretz deleted the rclpy_client_multiple_requests branch March 27, 2018 00:56

sloretz added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) labels Mar 27, 2018

dirk-thomas reviewed Feb 15, 2019

View reviewed changes

rclpy/rclpy/executors.py Show resolved Hide resolved

sloretz mentioned this pull request Feb 19, 2019

Dictionaries raise KeyError #276

Merged

wayneparrott mentioned this pull request Jun 2, 2022

Client only saves one callback at a time RobotWebTools/rclnodejs#858

Closed

YuanYuYuan pushed a commit to YuanYuYuan/rclpy that referenced this pull request Nov 12, 2025

Merge pull request ros2#170 from Carter12s/fix-service-servers-with-r…

8a03431

…osservice Fix service servers with rosservice

		executor.shutdown()


		def spin_until_future_complete(node, future):

Conversation

sloretz commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sloretz commented Jan 4, 2018 •

edited

Loading

sloretz commented Feb 7, 2018 •

edited

Loading

sloretz commented Mar 23, 2018 •

edited

Loading