Repository: gorakhargosh/watchdog

Issue:
Without `--wait`, shell subprocesses can deadlock / are not reaped
For example:

```
watchmedo shell-command -c 'cat' /some/file
```

The relevant code:

```
        self.process = subprocess.Popen(command, shell=True)                                    
        if self.wait_for_process:                                                               
            self.process.wait()    
```

One thing to note, the subprocess will inherit watchmedo's file descriptors, which maybe isn't desirable. (Or maybe it is?) In this case it means `cat` will never exit since stdin is inherited from `watchmedo`, which if `watchmedo` was run from an interactive shell (for example) is a TTY that may never be closed. So an endless stream of cats which never exit will be spawned.

But also, `wait(2)` is never called, so even if the subprocess does exit, it's never reaped. This accumulates zombie processes until `watchmedo` exits, at which point `init` will inherit those subprocesses and reap them. Of course that may never happen. You can see this with `ps -f` for example:

```
29547 ?        Sl     3:00  \_ /usr/bin/python /usr/local/bin/watchmedo shell-command -R -c [...]
19616 ?        Z      0:00      \_ [sh] <defunct>
19680 ?        Z      0:00      \_ [sh] <defunct>
19682 ?        Z      0:00      \_ [sh] <defunct>
19724 ?        Z      0:00      \_ [sh] <defunct>
19735 ?        Z      0:00      \_ [sh] <defunct>
19896 ?        Z      0:00      \_ [sh] <defunct>
20063 ?        Z      0:00      \_ [sh] <defunct>
20530 ?        Z      0:00      \_ [sh] <defunct>
23257 ?        Z      0:00      \_ [sh] <defunct>
```

One solution would be making `--wait` the default -- it's what I'd want personally for the use cases I have in mind.

Alternately, something needs to wait around for the child processes to exit. That could be a thread which calls `wait` which will block, or something like `epoll_wait` can do it in a non-blocking way.

Discussion:

Patch:
diff --git a/changelog.rst b/changelog.rst
index 747645c..7549fec 100644
--- a/changelog.rst
+++ b/changelog.rst
@@ -9,7 +9,8 @@ Unreleased
 2022-xx-xx • `full history <https://github.com/gorakhargosh/watchdog/compare/v2.1.8...HEAD>`__
 
 - [fsevents] Fix flakey test to assert that there are no errors when stopping the emitter.
-- [watchmedo] Make auto-restart restart the sub-process if it terminates. (`#896 <https://github.com/gorakhargosh/watchdog/pull/896>`__)
+- [watchmedo] Make ``auto-restart`` restart the sub-process if it terminates. (`#896 <https://github.com/gorakhargosh/watchdog/pull/896>`__)
+- [watchmedo] Avoid zombie sub-processes when running ``shell-command`` without ``--wait``. (`#405 <https://github.com/gorakhargosh/watchdog/issues/405>`__)
 - Thanks to our beloved contributors: @samschott, @taleinat
 
 2.1.8
diff --git a/src/watchdog/tricks/__init__.py b/src/watchdog/tricks/__init__.py
index 3303924..6e7eaf6 100644
--- a/src/watchdog/tricks/__init__.py
+++ b/src/watchdog/tricks/__init__.py
@@ -116,12 +116,14 @@ def __init__(self, shell_command=None, patterns=None, ignore_patterns=None,
         self.shell_command = shell_command
         self.wait_for_process = wait_for_process
         self.drop_during_process = drop_during_process
+
         self.process = None
+        self._process_watchers = set()
 
     def on_any_event(self, event):
         from string import Template
 
-        if self.drop_during_process and self.process and self.process.poll() is None:
+        if self.drop_during_process and self.is_process_running():
             return
 
         if event.is_directory:
@@ -151,6 +153,15 @@ def on_any_event(self, event):
         self.process = subprocess.Popen(command, shell=True)
         if self.wait_for_process:
             self.process.wait()
+        else:
+            process_watcher = ProcessWatcher(self.process, None)
+            self._process_watchers.add(process_watcher)
+            process_watcher.process_termination_callback = \
+                functools.partial(self._process_watchers.discard, process_watcher)
+            process_watcher.start()
+
+    def is_process_running(self):
+        return self._process_watchers or (self.process is not None and self.process.poll() is None)
 
 
 class AutoRestartTrick(Trick):
diff --git a/src/watchdog/utils/process_watcher.py b/src/watchdog/utils/process_watcher.py
index 800d7f8..f195764 100644
--- a/src/watchdog/utils/process_watcher.py
+++ b/src/watchdog/utils/process_watcher.py
@@ -1,5 +1,4 @@
 import logging
-import time
 
 from watchdog.utils import BaseThread
 
@@ -15,11 +14,10 @@ def __init__(self, popen_obj, process_termination_callback):
 
     def run(self):
         while True:
-            if self.stopped_event.is_set():
-                return
             if self.popen_obj.poll() is not None:
                 break
-            time.sleep(0.1)
+            if self.stopped_event.wait(timeout=0.1):
+                return
 
         try:
             self.process_termination_callback()


Test patch:
diff --git a/tests/test_0_watchmedo.py b/tests/test_0_watchmedo.py
index d9aaf3f..0f76e98 100644
--- a/tests/test_0_watchmedo.py
+++ b/tests/test_0_watchmedo.py
@@ -65,7 +65,7 @@ def test_kill_auto_restart(tmpdir, capfd):
     script = make_dummy_script(tmpdir)
     a = AutoRestartTrick([sys.executable, script])
     a.start()
-    time.sleep(5)
+    time.sleep(3)
     a.stop()
     cap = capfd.readouterr()
     assert '+++++ 0' in cap.out
@@ -74,6 +74,38 @@ def test_kill_auto_restart(tmpdir, capfd):
     # assert 'KeyboardInterrupt' in cap.err
 
 
+def test_shell_command_wait_for_completion(tmpdir, capfd):
+    from watchdog.events import FileModifiedEvent
+    from watchdog.tricks import ShellCommandTrick
+    import sys
+    import time
+    script = make_dummy_script(tmpdir, n=1)
+    command = " ".join([sys.executable, script])
+    trick = ShellCommandTrick(command, wait_for_process=True)
+    assert not trick.is_process_running()
+    start_time = time.monotonic()
+    trick.on_any_event(FileModifiedEvent("foo/bar.baz"))
+    elapsed = time.monotonic() - start_time
+    print(capfd.readouterr())
+    assert not trick.is_process_running()
+    assert elapsed >= 1
+
+
+def test_shell_command_subprocess_termination_nowait(tmpdir):
+    from watchdog.events import FileModifiedEvent
+    from watchdog.tricks import ShellCommandTrick
+    import sys
+    import time
+    script = make_dummy_script(tmpdir, n=1)
+    command = " ".join([sys.executable, script])
+    trick = ShellCommandTrick(command, wait_for_process=False)
+    assert not trick.is_process_running()
+    trick.on_any_event(FileModifiedEvent("foo/bar.baz"))
+    assert trick.is_process_running()
+    time.sleep(5)
+    assert not trick.is_process_running()
+
+
 def test_auto_restart_subprocess_termination(tmpdir, capfd):
     from watchdog.tricks import AutoRestartTrick
     import sys