Fix error in "stop()" preventing to "remove()" handler (Delgan#237)

CHANGELOG.rst

@@ -2,6 +2,7 @@
 =============
 
 - Fix the filter function listing files for  ``retention`` being too restrictive, it now matches files based on the pattern ``"basename(.*).ext(.*)"`` (`#229 <https://github.com/Delgan/loguru/issues/229>`_).
+- Fix the impossibility to ``remove()`` a handler if an exception is raised while the sink' ``stop()`` function is called (`#237 <https://github.com/Delgan/loguru/issues/237>`_).
 
 
 `0.4.1`_ (2020-01-19)

loguru/_logger.py

@@ -979,22 +979,23 @@ def remove(self, handler_id=None):
         with self._core.lock:
             handlers = self._core.handlers.copy()
 
+            if handler_id is not None and handler_id not in handlers:
+                raise ValueError("There is no existing handler with id %d" % handler_id) from None
+
             if handler_id is None:
-                for handler in handlers.values():
-                    handler.stop()
-                handlers.clear()
+                handler_ids = list(handlers.keys())
             else:
-                try:
-                    handler = handlers.pop(handler_id)
-                except KeyError:
-                    raise ValueError(
-                        "There is no existing handler with id %d" % handler_id
-                    ) from None
-                handler.stop()
+                handler_ids = [handler_id]
 
-            levelnos = (h.levelno for h in handlers.values())
-            self._core.min_level = min(levelnos, default=float("inf"))
-            self._core.handlers = handlers
+            for handler_id in handler_ids:
+                handler = handlers.pop(handler_id)
+
+                # This needs to be done first in case "stop()" raises an exception
+                levelnos = (h.levelno for h in handlers.values())
+                self._core.min_level = min(levelnos, default=float("inf"))
+                self._core.handlers = handlers
+
+                handler.stop()
 
     async def complete(self):
         """Wait for the end of the asynchronous tasks scheduled by coroutine handlers.

tests/test_remove.py

@@ -5,6 +5,14 @@
 import time
 
 
+class StopSinkError:
+    def write(self, message):
+        print(message, end="")
+
+    def stop(self):
+        raise Exception("Stop error")
+
+
 def test_remove_all(tmpdir, writer, capsys):
     file = tmpdir.join("test.log")
 
@@ -57,6 +65,39 @@ def test_remove_enqueue_filesink(tmpdir):
     assert file.read() == "1\n"
 
 
+def test_exception_in_stop_during_remove_one(capsys):
+    i = logger.add(StopSinkError(), catch=False, format="{message}")
+    logger.info("A")
+    with pytest.raises(Exception, match=r"Stop error"):
+        logger.remove(i)
+    logger.info("Nope")
+
+    out, err = capsys.readouterr()
+
+    assert out == "A\n"
+    assert err == ""
+
+
+def test_exception_in_stop_not_caught_during_remove_all(capsys):
+    logger.add(StopSinkError(), catch=False, format="{message}")
+    logger.add(StopSinkError(), catch=False, format="{message}")
+
+    with pytest.raises(Exception, match=r"Stop error"):
+        logger.remove()
+
+    logger.info("A")
+
+    with pytest.raises(Exception, match=r"Stop error"):
+        logger.remove()
+
+    logger.info("Nope")
+
+    out, err = capsys.readouterr()
+
+    assert out == "A\n"
+    assert err == ""
+
+
 def test_invalid_handler_id_value(writer):
     logger.add(writer)