8000 Refuse handlers if the child watcher has no loop attached by vxgmichel · Pull Request #391 · python/asyncio · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

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

Closed
wants to merge 7 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f348122
Refuse handlers if the child watcher has no loop attached
Aug 2, 2016
f7a5259
Warn when a child watcher with pending handlers looses its loop
Aug 2, 2016
9ab6168
Ignore or catch warnings from f7a5259 in the tests
Aug 2, 2016
d4b53cb
Fix test on windows (broke in 9ab6168)
Aug 2, 2016
256e713
Use assertWarnsRegex for test_set_loop_race_condition
Sep 15, 2016
99148c2
Add test_add_child_handler_with_no_loop_attached
Sep 15, 2016
08190f5
Update subprocess tests for FastChildWatcher
Sep 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update subprocess tests for FastChildWatcher
  • Loading branch information
Vincent Michel committed Sep 20, 2016
commit 08190f560b010e834ddc216fd100eea78f251cec
8 changes: 5 additions & 3 deletions tests/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,9 +433,11 @@ def kill_running():
# the transport was not notified yet
self.assertFalse(killed)

# Clear the handlers for FastChildWatcher to avoid a warning.
# Is that a bug?
if sys.platform != 'win32':
# Unlike SafeChildWatcher, FastChildWatcher does not pop the
# callbacks if waitpid() is called elsewhere. Let's clear them
# manually to avoid a warning when the watcher is detached.
if sys.platform != 'win32' and \
isinstance(self, SubprocessFastWatcherTests):
asyncio.get_child_watcher()._callbacks.clear()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def test_popen_error(self):
Expand Down
0