New test failure with Python 3.6

[wjwwood]

I'm assuming this is related to something new in Python 3.6, since some asyncio stuff changed in that version. This is the new test failure after updating the macOS CI machines:

http://ci.ros2.org/job/ci_osx/1720/testReport/junit/(root)/test_interrupt_asynchronous_launcher/test_interrupt_asynchronous_launcher/

Traceback (most recent call last):
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/launch/launch/launch/launcher.py", line 411, in run
    self.launcher.launch()
  File "/Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/launch/launch/launch/launcher.py", line 96, in launch
    raise e.exception
  File "/Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/launch/launch/launch/launcher.py", line 127, in _run
    await self._spawn_process(index)
  File "/Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/launch/launch/launch/launcher.py", line 351, in _spawn_process
    **kwargs)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/base_events.py", line 1190, in subprocess_exec
    bufsize, **kwargs)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 194, in _make_subprocess_transport
    self._child_watcher_callback, transp)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 858, in add_child_handler
    "Cannot add child handler, "
RuntimeError: Cannot add child handler, the child watcher does not have a loop attached

I looked into the problem for a while, but I'm not sure why this error is happening. I think the loop (main thread loop) should be attached to the child watcher, but maybe I'm missing something.

@dirk-thomas do you have any idea about this issue?

[wjwwood]

@dirk-thomas can you clarify what this comment is talking about:

launch/launch/launch/launcher.py

Line 105 in 3b990bc

# the watcher must be reset otherwise a repeated invocation fails inside asyncio

        if os.name != 'nt':
            # the watcher must be reset otherwise a repeated invocation fails inside asyncio
            asyncio.get_event_loop_policy().set_child_watcher(None)

It seems like asyncio is upset that the child watcher (in this case None) doesn't have a loop attached. Do you remember why exactly we're doing this?

[dirk-thomas]

When you want to launch a launch file twice (basically after it finished launch it again) it fails without this line.

[wjwwood]

Hmm, either way, removing this line doesn't fix this issue. Do you know of any other places in the code where we are setting up or changing a child watcher? I'm not finding anything else by searching.

[dhood]

FWIW I saw that someone else ran into this issue and fixed it with https://github.com/neovim/python-client/pull/241/files (not sure how that would work if we need it to be None)

[wjwwood]

I'll try to adapt that change, thanks @dhood.

[wjwwood]

I can get all of the tests to run individually, but I cannot get past the tests running together through ctest.

I am using this patch:

diff --git a/launch/launch/launcher.py b/launch/launch/launcher.py
index 757affe..a627c55 100644
--- a/launch/launch/launcher.py
+++ b/launch/launch/launcher.py
@@ -101,7 +101,7 @@ class DefaultLauncher(object):
         loop.close()
         with self.loop_lock:
             self.loop = None
-        if os.name != 'nt':
+        if os.name != 'nt' and isinstance(threading.current_thread(), threading._MainThread):
             # the watcher must be reset otherwise a repeated invocation fails inside asyncio
             asyncio.get_event_loop_policy().set_child_watcher(None)
 
@@ -398,6 +398,11 @@ class AsynchronousLauncher(threading.Thread):
     def __init__(self, launcher):
         super(AsynchronousLauncher, self).__init__()
         self.launcher = launcher
+        loop = asyncio.get_event_loop()
+        if loop.is_closed():
+            loop = asyncio.new_event_loop()
+            asyncio.set_event_loop(loop)
+        asyncio.get_child_watcher().attach_loop(loop)
 
     def terminate(self):
         self.launcher.interrupt_launch()

But I always get this error when running the tests via ctest:

======================================================================
ERROR: test_interrupt_default_launcher.test_interrupt_default_launcher
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/Users/william/ros2_ws/src/ros2/launch/launch/test/test_interrupt_default_launcher.py", line 54, in test_interrupt_default_launcher
    launcher.launch()
  File "/Users/william/ros2_ws/src/ros2/launch/launch/launch/launcher.py", line 89, in launch
    asyncio.set_event_loop(self.loop)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/events.py", line 676, in set_event_loop
    get_event_loop_policy().set_event_loop(loop)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 1051, in set_event_loop
    self._watcher.attach_loop(loop)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 806, in attach_loop
    self._do_waitpid_all()
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 876, in _do_waitpid_all
    self._do_waitpid(pid)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 910, in _do_waitpid
    callback(pid, returncode, *args)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/unix_events.py", line 213, in _child_watcher_callback
    self.call_soon_threadsafe(transp._process_exited, returncode)
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/base_events.py", line 619, in call_soon_threadsafe
    self._check_closed()
  File "/usr/local/Cellar/python3/3.6.0/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/base_events.py", line 357, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
-------------------- >> begin captured logging << --------------------
asyncio: DEBUG: Using selector: KqueueSelector
asyncio: WARNING: Unknown child process pid 65383, will report returncode 255
--------------------- >> end captured logging << ---------------------

Which, looking at the code, makes no sense to me:

launch/launch/launch/launcher.py

Line 87 in 3b990bc

asyncio.set_event_loop(self.loop)

As the very previous line does self.loop = asyncio.new_event_loop().

So, I guess we could either:

• Let someone else tackle this to see if I'm missing something obvious or
• Isolate the tests with subprocess or multiprocessing or
• Not allow calling .launch() from a thread, i.e. get rid of AsyncLauncher

I'm leaning towards the last option. Base on this SO:

http://stackoverflow.com/a/28917653/671658

It looks like you ought not use subprocess async calls from outside the main thread unless you also have a running loop in the main thread, which is probably exactly not what is happening when you use asynclauncher.

Looking on input for how to move forward on this one.

[wjwwood]

So, we discussed this off-line (some days ago now) and I think the conclusion was just to not support AsyncLauncher, i.e. not support running launch in a non-main thread. This appears to work with some hackery in Python 3.5, but Python 3.6 reintroduces issues. This SO summarizes it:

Documentation says:

• You should have running event loop in the main thread.
• In the main thread please call asyncio.get_child_watcher() at start of program.

After that you may create subprocess from non-main thread.

-- http://stackoverflow.com/a/28917653/671658

Since Python 3.6 is supposedly the first "stable" release of asyncio, I'm inclined to go along with it and say that launch only works if you meet the above criteria as well, even with Python 3.5. This means the original purpose of AsyncLauncher doesn't really work anymore. My understanding of the idea behind it was that you'd launch in a separate thread and do other things in the main python thread. This would only work if those "other things" was running an asyncio loop.

I propose we just remove AsyncLauncher and the related tests until someone asks specifically for this feature. @ros2/team can you guys please +1/-1 this proposal? Thanks.

[dirk-thomas]

Since we don't use it at the moment +1 to remove this.

[wjwwood]

Ok, I haven't gotten any feedback on this, so I'm going to proceed with a pr to remove this feature/test.

[clalancette]

Yeah, I'm with Dirk, if we aren't using it, let's just remove it, and we'll figure out another solution in the future if we need something similar.

[wjwwood]

@dirk-thomas sorry, you comment didn't show up (email or on the webpage) until I posted my comment 😢. Some strange notification issues going on I think...

[mikaelarguedas]

👍 for removing it for now and see if a critical use case shows up requiring us to implement an alternative