See ros2/launch_ros#169 for steps to reproduce the issue.

Actually, I'm not sure if this is the right way to handle the problem.

You can currently emit an event thread safely if you want, e.g.:

Reading the code I'm not sure how the two lines you pointed out @jacobperron could ever be run concurrently, but maybe I'm not using my imagination properly :)

Reading the code I'm not sure how the two lines you pointed out @jacobperron could ever be run concurrently, but maybe I'm not using my imagination properly :)

Looking again, I might be wrong about the reason for the hang.

It looks like is_idle evaluates to False on this line:

Which means either the queue is not empty or there are still futures to wait on. But when we get to processing events (from the queue) the queue is empty and we block forever on this line:

It looks like the future created here is never marked as done, even though logging suggests the function is run to completion:

https://github.com/ros2/launch_ros/blob/8c05ebb4be52dcd360e47694875643e4f53a010e/launch_ros/launch_ros/actions/load_composable_nodes.py#L173

It looks like the future created here is never marked as done, even though logging suggests the function is run to completion:

https://github.com/ros2/launch_ros/blob/8c05ebb4be52dcd360e47694875643e4f53a010e/launch_ros/launch_ros/actions/load_composable_nodes.py#L173

mmmm, so run_in_executor is using the default executor, which seems to actually be multi-threaded (reference needed here).
So, maybe that's the problem (?) ...

reference needed here

Reference found: https://github.com/python/cpython/blob/374d998b507d34a6c0a3816a163926a8ba0c483f/Lib/asyncio/base_events.py#L808-L812.
IMO, that's the problem

Yeah, the docs say the default executor must be a ThreadPoolExecutor.

What I think it's happening is the following:

• There's no more events to be processed
• We are waiting for some completion feature to be done
• the launch service awaits for a new event instead of waiting for the remaining completion futures to be done:

  launch/launch/launch/launch_service.py

  Line 341 in 657745c

  process_one_event_task = this_loop.create_task(self._process_one_event())

  
  

  launch/launch/launch/launch_service.py

  Lines 271 to 273 in 657745c

  	async def _process_one_event(self) -> None:
  	next_event = await self.__context._event_queue.get()
  	await self.__process_event(next_event)

  
  Thus, it waits for ever.

Easy hack: Using get with a timeout.
Not ideal, but I will need to think for real to figure out a better solution 😂.

Yeah, the docs say the default executor must be a ThreadPoolExecutor.

I like looking at the code 😂

Easy hack: Using get with a timeout.

Unfortunately, asyncio.Queue.get() does not have a timeout argument..

That was my initial theory too but the shutdown event didn’t clear the hang.

A better solution would be to have a noop event like “check for idle” or “future completed” that would cause get to return and would be posted after the future is completed.

I think I have figured out a fix (it works well in my mind at least):

The issue:

_is_iddle is returning true if there is either a future available or an event has not be processed. But we are then blocking on getting a new event (see 1 and 2).

We could rather do the following:

• Collect all futures like here.
• Wait on the first completed future like here.

If we always do that, not only after shutdown has happened, we should never run into the race (as we would check again _is_idle after any future has completed).

@ivanpauno how do you wait on events in the queue and the incomplete futures at the same time?

@ivanpauno how do you wait on events in the queue and the incomplete futures at the same time?

Using asyncio.wait with asyncio.FIRST_COMPLETED condition:

IIUC, that allows you to wait on a collection of awaitables at the same time, and it will return when one of them is completed.
In that way, we can recheck if the launch service is idle or not after any future is completed.

We should take care of not processing another event if wait returned for another reason, and not because one event was processed. But that can be checked.

Ok, so the set of things being waited on in each loop would be await process_one_event() + the context's futures + the action futures? Some of the logic might need to change, but it sounds like it should work.

Sounds good to me. I'll give it a try.

Almost seems to work; now I get stuck in the inner shutdown loop. Maybe I've hit a different bug related to shutdown, but I don't understand why there is a continue here:

What I see is this condition evaluates to True:

Then we call _shutdown and continue. And the next iteration, _is_idle() evaluates to False (I guess because the shutdown causes another event to be added to the queue):

Then we get stuck waiting to get something from the queue and repeatedly see this debug:

I guess this logic is broken (I don't know under what circumstance this is True):

If I remove the continue, my test works as expected, but maybe there are other unintended consequences.

Then we call _shutdown and continue. And the next iteration, _is_idle() evaluates to False (I guess because the shutdown causes another event to be added to the queue):

That continue ensure that self.__shutting_down is true:

At least I think that's the case. Maybe that's no longer the case.

@jacobperron can you push that code somewhere?

I've reverted the original changes and instead refactored the run_async loop to wait on both queued events and futures (92bfb44). I also added some logic to wait until the process_one_event task is done or a timeout occurs (19e0d68).

This solves my original issue, but now the test_launch_service.py tests are hangning 🙃

The test is emitting an event from a thread and it gets get stuck here:

Launch service call stack ends up here:

and waiting forever for the result

I guess the issue is that we need to await always in the loop, in the case that there are no futures or events to wait on I'm running a busy loop.

@ivanpauno @wjwwood Okay, I think I've got it now (1118883). PTAL at the latest changes.

I did a minor refactor 41a31d2 and added an guard against a pending task during shutdown f33e68e. Let me know if there's anything else.

Great finding! LGTM too with green CI.

Testing packages recursively depending on launch (except ros1_bridge):

• Linux
• Linux-aarch64
• macOS
• Windows

The Rpr job is failing due to an infrastructure issue. Since the failure is unrelated to this change and CI is green, I'll go ahead and merge.

Thanks for the reviews everyone!

I've decided not to backport this to Eloquent or Dashing since there's been a lot of changes since they were last touched.