add wait_for_service() and service_is_ready() to Client #222

wjwwood · 2016-05-27T03:30:34Z

Replacement for #221

jacquelinekay · 2016-06-13T22:29:07Z

rclcpp/src/rclcpp/graph_listener.cpp

+    }  // release the nodes_ lock
+
+    // Wait for graph changes or interrupt.
+    ret = rcl_wait(&wait_set_, -1);  // block for ever until one of the


Incomplete comment here.

wjwwood · 2016-06-15T19:19:51Z

This needs review. @jacquelinekay already reviewed in some while helping me find and fix a bug.

A few things I want to highlight and to "request comments" about:

Exceptions:
- I have been trying in this pull request to be better about throwing specific exceptions.
- However, I'm not sure how granular to make them, so I'm open to suggestion on how to adjust those.
- Also, I'm not sure how best to organize the exceptions: all in one header (rclcpp/exceptions.hpp) or all in the files they're related to (Node specific exceptions in rclcpp/node.hpp) or a mixture of both?
  - Currently I have a mixture of both, with some generic RCL exceptions and help functions in the rclcpp/exceptions.hpp header, and other exceptions in node.hpp and graph_listener.hpp.
GraphListener:
- The design is such that there is a single GraphListener with a single thread for all nodes in the given context.
- This allows us to wait on all graph changes from all nodes in a single thread.
- Waiting on graph changes in a thread allows functions like wait_for_service to work without an executor spinning.
- It is setup to only create the thread and wake-up in that thread if there are nodes which need to be notified of graph changes, so it's lazy in that way to conserve resources when not being used.
Testing:
- All the new code is tested via integration tests in test_communication, test_rclcpp, and rclcpp_examples.
- I wanted to test GraphListener in a unit test fashion, but I found it too difficult to do so since it takes a full Node into most of its arguments.
  - Mocking Node to simplify the GraphListener tests is too hard, or impossible without dirty hacks, see: https://github.com/ros2/rclcpp/blob/master/rclcpp/test/test_intra_process_manager.cpp#L23-L133
  - I have a plan on how to change the headers in rclcpp to make this easier, but I think that's out of scope for this pull request. However, adding narrower tests for these classes would hopefully be easier after the changes. I wanted to discuss them at the last meeting but we ran out of time.

I think that's all I wanted to highlight. Thanks in advance for reviews.

jacquelinekay · 2016-06-16T20:58:24Z

rclcpp/src/rclcpp/client.cpp

+    }
+    // server is not ready, loop if there is time left
+    time_to_wait = timeout - (std::chrono::steady_clock::now() - start);
+  } while (timeout < std::chrono::nanoseconds(0) || time_to_wait > std::chrono::nanoseconds(0));


What happens in this loop when the program receives SIGINT/CTRL-C?

That's a good question... Currently I think it will not wake up, because neither node_->wait_for_graph_change nor this loop will consider rclcpp::ok or be notified by the shutdown call or signal processor.

What do you think the behavior of this function should be in that case? I'll need to adjust the documentation to be something like "returns true if graph changed or false if the timeout expired or rclcpp::shutdown occurred". But then you can't easily tell which happened timeout or shutdown, though I guess if you check rclcpp::ok after getting false back from this function and be reasonably sure that was the cause or not.

I think it's fine to check for !rclcpp::ok() in the while predicate and return false in that case. A return value of false means "we stopped waiting and the client was not ready." If you want a finer level of granularity we could add error codes (client_ready, timeout, interrupted) but I don't think that's necessary right now.

Ok, I added this behavior in 25e17b0 and I added a test in ros2/system_tests@268f54a.

jacquelinekay · 2016-06-16T21:27:12Z

My major questions are about what happens in the do/while loop on CTRL-C, and the ownership of the Node pointer in Client. Otherwise it looks good.

jacquelinekay · 2016-06-17T17:06:34Z

+1

dirk-thomas · 2016-06-17T17:08:46Z

+1

this essentially protects the notify_guard_condition_

wjwwood · 2016-06-21T21:55:12Z

I rebased and refactored the thread synchronization.

…re static destruction

* add wait_for_service() and service_is_ready() to Client * fix compile on Linux (maybe Windows) * use visibility macros for Windows * prevent unreasonable uncrustify change * fixup comment * add GraphListener::is_shutdown() * disable copy on GraphListener * use weak_ptr<Node> in client, throw if invalid * ensure blocking wait_for_service wakes on rclcpp::shutdown/sigint * rethrow exceptions after reporting them in thread * lock ~Node() against notify_graph_change() this essentially protects the notify_guard_condition_ * adjust thread sync strategy * style * moving initialization of wait set around, fix double free * only fini wait set if started * use rclcpp::shutdown to ensure graph listener resources clean up before static destruction * uncrustify

* add missing documentation * remove unnecessary visibility and warning macros

wjwwood added the in progress Actively being worked on (Kanban column) label May 27, 2016

This was referenced May 27, 2016

add ability to wait for a service server to be available #221

Closed

implement wait_for_service function ros2/ros2#215

Closed

wjwwood force-pushed the wait_for_service2 branch 2 times, most recently from 642174d to 7f74cb9 Compare June 9, 2016 01:51

wjwwood changed the title ~~initial implementation of wait_for_service, WIP~~ initial implementation of wait_for_service Jun 9, 2016

wjwwood changed the title ~~initial implementation of wait_for_service~~ add wait_for_service() and service_is_ready() to Client Jun 9, 2016

wjwwood force-pushed the wait_for_service2 branch 2 times, most recently from 9a3afec to 760efde Compare June 9, 2016 21:29

jacquelinekay reviewed Jun 13, 2016
View reviewed changes

wjwwood self-assigned this Jun 15, 2016

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 15, 2016

jacquelinekay reviewed Jun 16, 2016
View reviewed changes

wjwwood force-pushed the wait_for_service2 branch from acab9e5 to 25e17b0 Compare June 17, 2016 01:49

wjwwood added 7 commits June 21, 2016 14:54

add wait_for_service() and service_is_ready() to Client

5b2e4f0

fix compile on Linux (maybe Windows)

f90205a

use visibility macros for Windows

ee3758c

prevent unreasonable uncrustify change

f10f1ad

fixup comment

42652f7

add GraphListener::is_shutdown()

cb86eca

disable copy on GraphListener

5ac6d45

wjwwood added 5 commits June 21, 2016 14:54

use weak_ptr<Node> in client, throw if invalid

f839119

ensure blocking wait_for_service wakes on rclcpp::shutdown/sigint

39ed385

rethrow exceptions after reporting them in thread

fa3d3e7

lock ~Node() against notify_graph_change()

5b3ff2c

this essentially protects the notify_guard_condition_

adjust thread sync strategy

b105fe6

wjwwood force-pushed the wait_for_service2 branch from a660203 to b105fe6 Compare June 21, 2016 21:54

wjwwood and others added 5 commits June 22, 2016 01:46

style

f19c22d

moving initialization of wait set around, fix double free

071dab8

only fini wait set if started

0b2d362

use rclcpp::shutdown to ensure graph listener resources clean up befo…

c22fac1

…re static destruction

uncrustify

2f486f4

wjwwood merged commit 5e2a76c into master Jun 23, 2016

wjwwood deleted the wait_for_service2 branch June 23, 2016 03:18

wjwwood removed the in review Waiting for review (Kanban column) label Jun 23, 2016

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Fix docs (ros2#222)

e48a445

* add missing documentation * remove unnecessary visibility and warning macros

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add wait_for_service() and service_is_ready() to Client #222

add wait_for_service() and service_is_ready() to Client #222

wjwwood commented May 27, 2016 •

edited

Loading

jacquelinekay Jun 13, 2016

wjwwood Jun 13, 2016

wjwwood commented Jun 15, 2016

jacquelinekay Jun 16, 2016

wjwwood Jun 16, 2016

jacquelinekay Jun 16, 2016

wjwwood Jun 17, 2016

jacquelinekay commented Jun 16, 2016

jacquelinekay commented Jun 17, 2016

dirk-thomas commented Jun 17, 2016

wjwwood commented Jun 21, 2016

add wait_for_service() and service_is_ready() to Client #222

add wait_for_service() and service_is_ready() to Client #222

Conversation

wjwwood commented May 27, 2016 • edited Loading

jacquelinekay Jun 13, 2016

Choose a reason for hiding this comment

wjwwood Jun 13, 2016

Choose a reason for hiding this comment

wjwwood commented Jun 15, 2016

jacquelinekay Jun 16, 2016

Choose a reason for hiding this comment

wjwwood Jun 16, 2016

Choose a reason for hiding this comment

jacquelinekay Jun 16, 2016

Choose a reason for hiding this comment

wjwwood Jun 17, 2016

Choose a reason for hiding this comment

jacquelinekay commented Jun 16, 2016

jacquelinekay commented Jun 17, 2016

dirk-thomas commented Jun 17, 2016

wjwwood commented Jun 21, 2016

wjwwood commented May 27, 2016 •

edited

Loading