Use wait_for_service after creating parameters_client

[dhood]

connects to ros2/rclcpp#356

I don't think it's necessary to do any waiting when calling node->set_parameters because FWIU there are no service calls involved (correct me if I'm wrong).
Also, now that we wait in so many places, the timeout has to be increased significantly (15 waits of 20s)

CI including other branches that add the implementation of wait_for_service and use it in demos:

• Linux
• Linux-aarch64
• macOS
• Windows

this should fix flaky tests e.g. http://ci.ros2.org/view/nightly/job/nightly_linux_repeated/744/testReport/junit/(root)/projectroot/gtest_local_parameters__rmw_fastrtps_cpp but it doesn't fix them completely yet (the test can hang while waiting for the service instead of the assertion being raised after max 20s )

[dhood]

I don't think that wait_for_service is the issue here anymore. I'll provide a more detailed description tomorrow but will merge the connected PRs tonight before the nightlies start to get fewer test failures in the demo_nodes_cpp package in the meantime.

[dhood]

OK, following up. This only happens with fastrtps (here's connext passing 100 times without any other modifications: ). It usually gets stuck waiting for the service response. The reason why I didn't think that it was related to the wait_for_service method of the parameters client is because sometimes this happens even after a successful call to the service, e.g.:

[ RUN      ] test_local_parameters__rmw_fastrtps_cpp.local_synchronous
Setting parameters
Listing parameters with recursive depth
Listing parameters with depth of 1
Listing parameters with depth of 2
Getting parameters
Getting nonexistent parameters
Listing parameters with recursive depth
Listing parameters with depth 100
Listing parameters with depth 1

<hangs>

It seems to wait for the Nth service call indefinitely and never receive it. This is fixed by ros2/rmw_fastrtps#143

Other times the service call is complete and the executor is trying to return success, but then it hangs. This is fixed by ros2/rmw_fastrtps#142

While investigating, I've fixed a potential memory error in ros2/rcl_interfaces#18 and also prevented another crash that occurs at shutdown with eProsima/Fast-DDS#134, but neither of those fix the hanging by themselves.

On my machine, which at this point has lots of zombie nodes lying around, to run the tests repeatedly eProsima/Fast-DDS#134 is needed to prevent crashing on teardown. On the buildfarm it didn't seem to be necessary though:

These jobs include this PR and both of the rmw_fastrtps PRs. It does NOT include the fastrtps PR or the rcl_interfaces PR. The local_parameters__rmw_fastrtps_cpp test was run 100 times checking for flakiness:

• Linux
• Linux-aarch64
• macOS
• Windows

(For the record, here are jobs that also include the fastrtps PR)
This PR, the fastrtps PR, and both of the rmw_fastrtps PRs:

• Linux
• Linux-aarch64
• macOS
• Windows

[dhood]

Redid the OSX job. This is rerunning local_parameters__rmw_fastrtps_cpp tests 100 times on OS X again, but this time including ros2/rcl_interfaces#18:
Since we're dealing with flaky tests it's hard to know for sure if that PR had anything to do with it, but regardless, the tests in local_paramters__rmw_fastrtps_cpp are much much less flaky now if they can pass 100 times.

This is repeating all of the test_rclcpp tests for fastrtps 100 times . The only failures are of type "service not available after waiting" but I'd consider that a separate issue that I won't investigate at this time.

In summary, with the other PRs allowing this PR to be tested thoroughly I'm confident in it and am putting this PR back in review.

[dirk-thomas]

Please move this line one line higher. Otherwise it separates the comment line from the line it is commenting.

[mikaelarguedas]

Nice that we can reduce it to 90!

[dhood]

CI in ros2/rmw_fastrtps#147 (comment) showed that local parameter tests don't need wait_for_service, it must just be the remote ones. Indeed, https://github.com/ros2/system_tests/pull/159/files only ever added the workaround for the remote tests.

I didn't realise that when the service constructor returns, it must be good to go (I figured there was middleware work going on asynchronously). I'll remove the wait_for_service calls from the local_paramters tests.

[dhood]

Nevermind, there may/may not still be middleware work going on asynchronously. Regardless, we decided that the wait_for_service calls do belong conceptually, so I restored them.

Potentially, adding these wait_for_service calls might now make these tests flaky if whatever causes the service not available after waiting error in the remote client-server tests is, for example, a race condition that we're now bringing into these tests. I don't think that should impact our action for now, just being verbose.

adding so many wait_for_service calls means the timeout has to be large to avoid timeouts. I opened ros2/rclcpp#360 to track that we should put timeouts on the parameter calls when available so that we don't waste 5min if one service call fails.

standard CI

• Linux
• Linux-aarch64
• macOS
• Windows (unrelated flaky tests)

repeating parameter tests 100 times:

• Linux
• Linux-aarch64
• macOS
• Windows