Add tips about service client and rate

[mjforan]

No description provided.

[fujitatomoya]

IMO I would like to keep those documents for the transition guidance and tutorials as simple as that. instead, i would add those note in https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Executors.html.

[mjforan]

IMO I would like to keep those documents for the transition guidance and tutorials as simple as that. instead, i would add those note in https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Executors.html.

I would not say these are intermediate concepts. As a matter of fact, the rate.sleep() design pattern is used in the first rospy tutorial.

What do you think about my shortened wording? It's easily digestible for complete beginners while serving as an important tip for developers coming from ROS 1.

[mjforan]

bump

[mjforan]

@fujitatomoya @clalancette bump

[fujitatomoya]

lgtm.

i will leave this @clalancette

[mjforan]

It has come to my attention that publishers can have a similar issue with deadlock. Doesn't that make the basic python tutorial incorrect, since it uses the publish method in a timer callback with a single-threaded executor?

[clalancette]

It has come to my attention that publishers can have a similar issue with deadlock. Doesn't that make the basic python tutorial incorrect, since it uses the publish method in a timer callback with a single-threaded executor?

That should not be the case, as far as we know. Publishers do not use the executor, and thus are not subject to the same deadlock. Can you give more details about the deadlock you are seeing?

[mjforan]

Interesting - I will do some more investigating and submit a separate bug report for my issue.

Anyways, this PR should be ready for @clalancette approval.

[mjforan]

@clalancette can you please take a look at this PR?

[clalancette]

This is still wrong; the path here needs to be updated (sorry, I made a mistake in my earlier suggestion).

But the closer we get to putting this in, the less I like it here. As a beginner user with this tutorial, what am I supposed to do about this warning? The tutorial just told me to use it, and then it is warning me against it. It doesn't make much sense to me.

Now that I think about this more, I think that if we want to do something here, we need to change the tutorial somehow so that it doesn't use this call, or at least uses it in a safe context.

What I'm going to suggest is that we just remove this line from this PR, so we can get the changes to the Migrating-Python-Packages in. Then we can consider what to do here separately.

Suggested change

	.. warning::
	Synchronous calls such as ``rclpy.spin_until_future_complete`` can cause deadlock.
	For more details see the :doc:`sync deadlock article <../Sync-Vs-Async>`.

[mjforan]

The tutorial just told me to use it, and then it is warning me against it.

Sure, but then if they use it in the wrong way it will break with no debug output. That's what the warning should mean, "be careful how you use this". I still think my earlier explicit wording was better; we can shorten it to just say "Do not use this in a ROS callback" with a link to the other article for further reading.

I don't want to remove this entirely because then there will be no mention of the issue in any of the tutorials. This one-line warning will not be too confusing for beginners.

Obviously the final call is yours to make, so if this is too much go ahead and just merge the other file.

[mjforan]

@clalancette

[mjforan]

@clalancette @fujitatomoya Is there anything blocking merge? I think the changes are simple enough at this point.

[fujitatomoya]

@mjforan this needs to rebase.

@clalancette can you check your unresolved comments?

[mjforan]

@clalancette

[clalancette]

After a long time here (sorry about that), I did a bit of updating and simplifying. I think this is in an OK state now. I'm going to approve and merge this, and also backport to all of the supported distributions. Thanks @mjforan for your patience.