Skip to content
This repository has been archived by the owner. It is now read-only.

Refuse handlers if the child watcher has no loop attached #391

Closed
wants to merge 7 commits into from

Conversation

@vxgmichel
Copy link

vxgmichel commented Aug 2, 2016

This PR follows up on the discussion from issue #390:

  • f348122 fixes the issue
  • f7a5259 is a proposal and can easily be discarded

This has been tested against the example from issue #390.

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Sep 12, 2016

Will push this before b2.

Copy link
Member

1st1 left a comment

Would very much want this to land in 3.6. Please address the comments.

# Clear the handlers for FastChildWatcher to avoid a warning.
# Is that a bug?
if sys.platform != 'win32':
asyncio.get_child_watcher()._callbacks.clear()

This comment has been minimized.

Copy link
@1st1

1st1 Sep 15, 2016

Member

This is something new... Is there any reason you do this as part of this PR?

This comment has been minimized.

Copy link
@vxgmichel

vxgmichel Sep 15, 2016

Author

Yes, otherwise a warning from f7a5259 is generated. Weirdly enough it only happens with the FastChildWatcher. There might be a better way to address this issue though.

This comment has been minimized.

Copy link
@vxgmichel

vxgmichel Sep 15, 2016

Author

I just had a deeper look and we can get rid of this addition by fixing FastChildWatcher (using some SafeChildWatcher code):

diff --git a/asyncio/unix_events.py b/asyncio/unix_events.py
index 54cfc75..5ffa645 100644
--- a/asyncio/unix_events.py
+++ b/asyncio/unix_events.py
@@ -937,7 +937,15 @@ def _do_waitpid_all(self):
                 pid, status = os.waitpid(-1, os.WNOHANG)
             except ChildProcessError:
                 # No more child processes exist.
-                return
+                if not self._callbacks:
+                    return
+                # Some child processes are already reaped
+                # (may happen if waitpid() is called elsewhere).
+                pid = next(iter(self._callbacks))
+                returncode = 255
+                logger.warning(
+                    "Unknown child process pid %d, will report returncode 255",
+                    pid)
             else:
                 if pid == 0:
                     # A child process is still alive.

This also allows us to get rid of some complexity in test_sigchld_child_reaped_elsewhere:

diff --git a/tests/test_unix_events.py b/tests/test_unix_events.py
index 6cf4417..b4a6d80 100644
--- a/tests/test_unix_events.py
+++ b/tests/test_unix_events.py
@@ -1318,12 +1318,7 @@ def test_sigchld_child_reaped_elsewhere(self, m):
         with self.ignore_warnings:
             self.watcher._sig_chld()

-        if isinstance(self.watcher, asyncio.FastChildWatcher):
-            # here the FastChildWatche enters a deadlock
-            # (there is no way to prevent it)
-            self.assertFalse(callback.called)
-        else:
-            callback.assert_called_once_with(58, 255)
+        callback.assert_called_once_with(58, 255)

     @waitpid_mocks
     def test_sigchld_unknown_pid_during_registration(self, m):

All the tests run OK after those modifications. Let me know which solution you prefer.

This comment has been minimized.

Copy link
@1st1

1st1 Sep 15, 2016

Member

Not so sure about this solution. Let's keep the current hack-ish code.

@@ -1396,7 +1397,11 @@ def test_set_loop_race_condition(self, m):
with mock.patch.object(
old_loop, "remove_signal_handler") as m_remove_signal_handler:

self.watcher.attach_loop(None)

This comment has been minimized.

Copy link
@1st1

1st1 Sep 15, 2016

Member

How about using with self.assertWarnsRegexp?

This comment has been minimized.

Copy link
@vxgmichel

vxgmichel Sep 15, 2016

Author

Done, it's much cleaner now!

@@ -898,6 +905,12 @@ def __exit__(self, a, b, c):

def add_child_handler(self, pid, callback, *args):
assert self._forks, "Must use the context manager"

if self._loop is None:

This comment has been minimized.

Copy link
@1st1

1st1 Sep 15, 2016

Member

Can we add a test for this?

This comment has been minimized.

Copy link
@vxgmichel
@vxgmichel vxgmichel force-pushed the vxgmichel:issue-390 branch from 80b2c33 to 99148c2 Sep 15, 2016
@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Sep 15, 2016

@asvetlov Could you please help me with reviewing this PR?

@asvetlov

This comment has been minimized.

Copy link

asvetlov commented Sep 19, 2016

Sorry for a long delay.
LGTM.
if win32 looks tricky but I can live with it.

@asvetlov

This comment has been minimized.

Copy link

asvetlov commented Sep 19, 2016

Looks like I should update my pytest fixtures for pytest-aiohttp by explicitly attaching/detaching test loop.
It's really nice catch. By writing fixtures for aiohttp I totally forgot about subprocess quirks on UNIX.
Honestly I almost never run subprocesses in my asyncio-based code but raising an explicit error is much better than just hanging in worst case.
Casual usage without default loop disabling is not affected as I see.

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Sep 19, 2016

@asvetlov Andrew, I'm on vacation, and I can't commit this myself right now. If you have time, could you please commit this pr (and merge it to cpython 3.5/3.6/default)?

@1st1
1st1 approved these changes Sep 26, 2016
1st1 added a commit that referenced this pull request Oct 5, 2016
@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Oct 5, 2016

Committed by hand. Thanks, Vincent!

@1st1 1st1 closed this Oct 5, 2016
@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Oct 5, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.