Repository: gorakhargosh/watchdog

Issue:
Remove emitters which failed to start
This is one corner case.

When an emitter is added it is automatically started... but if fails to start, it remains added.

Now, if someone add again an emitter for the same watch, the emitter is not retried and remains stopped.

Not sure if watchdog should handle this case, or users should manually unsubscribe an emitter on failures

Thanks!


Discussion:
BoboTiG: Cc @danilobellini.
rrzaripov: @BoboTiG LGFM
rrzaripov: @BoboTiG some Windows tests fails, but looks like not a bug, just rework\update need here.

Patch:
diff --git a/changelog.rst b/changelog.rst
index a738c1c..b71bd0e 100644
--- a/changelog.rst
+++ b/changelog.rst
@@ -18,6 +18,7 @@ API changes
 - Fixed  a race condition crash when a directory is swapped for a file. 
 - Fixed the way we re-raise ``OSError``.
 - Fixed the path separator used in watchmedo. 
+- Remove emitters which failed to start.
 - We now generate sub created events only if ``recursive=True``.
 - Security fix in watchmedo: use ``yaml.safe_load()`` instead of ``yaml.load()``
 - Use ``scandir`` to save memory.
diff --git a/src/watchdog/observers/api.py b/src/watchdog/observers/api.py
index 01fc8a1..98d63a9 100644
--- a/src/watchdog/observers/api.py
+++ b/src/watchdog/observers/api.py
@@ -248,8 +248,12 @@ def emitters(self):
         return self._emitters
 
     def start(self):
-        for emitter in self._emitters:
-            emitter.start()
+        for emitter in self._emitters.copy():
+            try:
+                emitter.start()
+            except Exception:
+                self._remove_emitter(emitter)
+                raise
         super(BaseObserver, self).start()
 
     def schedule(self, event_handler, path, recursive=False):


Test patch:
diff --git a/tests/test_emitter.py b/tests/test_emitter.py
index 3e414cc..792267d 100644
--- a/tests/test_emitter.py
+++ b/tests/test_emitter.py
@@ -41,7 +41,7 @@
         InotifyFullEmitter,
     )
 elif platform.is_darwin():
-    pytestmark = pytest.mark.skip("FIXME: It is a matter of bad comparisons between bytes and str.")
+    pytestmark = pytest.mark.skip("FIXME: issue #546.")
     from watchdog.observers.fsevents2 import FSEventsEmitter as Emitter
 elif platform.is_windows():
     from watchdog.observers.read_directory_changes import (
diff --git a/tests/test_observer.py b/tests/test_observer.py
index 26229cc..f40570d 100644
--- a/tests/test_observer.py
+++ b/tests/test_observer.py
@@ -115,3 +115,30 @@ def test_2_observers_on_the_same_path(observer, observer2):
 
     observer2.schedule(None, '')
     assert len(observer2.emitters) == 1
+
+
+def test_start_failure_should_not_prevent_further_try(monkeypatch, observer):
+    observer.schedule(None, '')
+    emitters = observer.emitters
+    assert len(emitters) == 1
+
+    # Make the emitter to fail on start()
+
+    def mocked_start():
+        raise OSError()
+
+    emitter = next(iter(emitters))
+    monkeypatch.setattr(emitter, "start", mocked_start)
+    with pytest.raises(OSError):
+        observer.start()
+    # The emitter should be removed from the list
+    assert len(observer.emitters) == 0
+
+    # Restoring the original behavior should work like there never be emitters
+    monkeypatch.undo()
+    observer.start()
+    assert len(observer.emitters) == 0
+
+    # Re-schduling the watch should work
+    observer.schedule(None, '')
+    assert len(observer.emitters) == 1
diff --git a/tests/test_observers_polling.py b/tests/test_observers_polling.py
index f94041a..109163b 100644
--- a/tests/test_observers_polling.py
+++ b/tests/test_observers_polling.py
@@ -65,12 +65,14 @@ def event_queue():
 @pytest.fixture
 def emitter(event_queue):
     watch = ObservedWatch(temp_dir, True)
-    yield Emitter(event_queue, watch, timeout=0.2)
+    em = Emitter(event_queue, watch, timeout=0.2)
+    em.start()
+    yield em
+    em.stop()
 
 
 def test___init__(event_queue, emitter):
     SLEEP_TIME = 0.4
-    emitter.start()
 
     sleep(SLEEP_TIME)
     mkdir(p('project'))
@@ -165,7 +167,6 @@ def test___init__(event_queue, emitter):
 def test_delete_watched_dir(event_queue, emitter):
     SLEEP_TIME = 0.4
 
-    emitter.start()
     rm(p(''), recursive=True)
 
     sleep(SLEEP_TIME)
diff --git a/tests/test_observers_winapi.py b/tests/test_observers_winapi.py
index 52367e3..4be47c5 100644
--- a/tests/test_observers_winapi.py
+++ b/tests/test_observers_winapi.py
@@ -59,14 +59,14 @@ def event_queue():
 @pytest.fixture
 def emitter(event_queue):
     watch = ObservedWatch(temp_dir, True)
-    yield WindowsApiEmitter(event_queue, watch, timeout=0.2)
+    em = WindowsApiEmitter(event_queue, watch, timeout=0.2)
+    yield em
+    em.stop()
 
 
-def test___init__():
+def test___init__(event_queue, emitter):
     SLEEP_TIME = 2
 
-    emitter.start()
-
     sleep(SLEEP_TIME)
     mkdir(p('fromdir'))
 
@@ -81,10 +81,10 @@ def test___init__():
     #   * unordered
     #   * non-unique
     # A multiset! Python's collections.Counter class seems appropriate.
-    expected = {[
+    expected = {
         DirCreatedEvent(p('fromdir')),
         DirMovedEvent(p('fromdir'), p('todir')),
-    ]}
+    }
 
     got = set()