Fix osrf.py_common.process_utils.get_loop() implementation

[hidmic]

On Windows, avoid loops closing during garbage collection and reuse existing ones if possible.

Connected to ros2/launch#476.

[clalancette]

From a naive view, it looks like this shouldn't be necessary; the first time we come in here, the TLS loop_has_been_setup will be False, we'll unconditionally create the ProactorEventLoop, and then set loop_has_been_setup to True. Subsequent invocations should always just use the earlier get_event_loop. Could you explain a bit more why this is needed?

[hidmic]

CI up to launch, test_launch_ros, test_communication, and osrf_pycommon:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

From a naive view, it looks like this shouldn't be necessary; the first time we come in here, the TLS loop_has_been_setup will be False, we'll unconditionally create the ProactorEventLoop, and then set loop_has_been_setup to True. Subsequent invocations should always just use the earlier get_event_loop. Could you explain a bit more why this is needed?

Thing is, the event loop for the current thread may have already been set (e.g. through regular asyncio API). On Unix, we simply use the one set. But on Windows, we were always overriding it. If my analysis is correct (and I believe so, because I don't get hangs anymore), this creates a fun scenario if the event loop that was previously set was an asyncio.ProactorEventLoop itself. Once we override it, if there are no references to it, it'll eventually be garbage collected. When it is garbage collected, and Python's garbage collector runs in the main thread, that loop gets closed, and boom! Signal wakeups are disabled, affecting the other asyncio.ProactorEventLoop instance that is running.

IMHO, this is both a bug in asyncio and a not so adequate use of asyncio in launch. If you look at the upcoming deprecations in asyncio, it's clear event loops are meant to be global, at most one-time set instances.

[ivanpauno]

When it is garbage collected, and Python's garbage collector runs in the main thread, that loop gets closed, and boom! Signal wakeups are disabled, affecting the other asyncio.ProactorEventLoop instance that is running.

omg, wonderful finding 😅

[clalancette]

Thing is, the event loop for the current thread may have already been set (e.g. through regular asyncio API). On Unix, we simply use the one set. But on Windows, we were always overriding it. If my analysis is correct (and I believe so, because I don't get hangs anymore), this creates a fun scenario if the event loop that was previously set was an asyncio.ProactorEventLoop itself. Once we override it, if there are no references to it, it'll eventually be garbage collected. When it is garbage collected, and Python's garbage collector runs in the main thread, that loop gets closed, and boom! Signal wakeups are disabled, affecting the other asyncio.ProactorEventLoop instance that is running.

Wow, deep stuff. Good find. So am I right in thinking that the main fix here is the close of the old loop and the open of a new ProactorEventLoop under Windows? If so, it might be worthwhile to put a comment explaining that, just because it is non-obvious from the context.

[wjwwood]

I didn't know the context for the issue. I just assumed the closing in the gc might be an issue if it is actually a singleton but didn't ask after it. Nice to hear the explanation. I agree a comment detailing why this code is like this would be useful.

[hidmic]

FYI https://bugs.python.org/issue42913.