-
Notifications
You must be signed in to change notification settings - Fork 225
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.
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
Refactor client for multiple requests #170
Conversation
1182f65
to
6a7d6a9
Compare
6a7d6a9
to
93bbea4
Compare
ec14788
to
f60ec40
Compare
Mentioned it to @sloretz offline but writing it here for book-keeping. |
rclpy/rclpy/client.py
Outdated
""" | ||
sequence_number = _rclpy.rclpy_send_request(self.client_handle, req) | ||
future = Future() | ||
self._pending_requests[sequence_number] = future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to be defensive should this check / ensure that sequence_number
is not already in the dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check in ece3781
rclpy/rclpy/client.py
Outdated
future = Future() | ||
self._pending_requests[sequence_number] = future | ||
|
||
def remove_pending_request(future): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is bf87341 what you have in mind?
# The request was cancelled | ||
pass | ||
else: | ||
future._set_executor(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird that the executor is calling a "private" method on the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
96b94f7
to
1ee7e79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, with some comments
# The request was cancelled | ||
pass | ||
else: | ||
future._set_executor(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -54,3 +54,14 @@ def spin(node): | |||
executor.spin_once() | |||
finally: | |||
executor.shutdown() | |||
|
|||
|
|||
def spin_until_future_complete(node, future): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a doc block, but so are many existing functions. It would be nice to add, but not required I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in cf17f38
Running CI since it's been a while and cf17f38 was added. Edit: CI looks good |
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 aSingleThreadedExecutor
. 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
projectroot.test_api_srv_composition_client_first__rmw_fastrtps_cpp
nightly_win_deb
http://ci.ros2.org/view/nightly/job/nightly_win_deb/764nightly_win_rep
http://ci.ros2.org/view/nightly/job/nightly_win_rel/705