Fix bugged file sink if error during retention/compression (Delgan#238)

CHANGELOG.rst

@@ -3,6 +3,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>`_).
+- Fix file sink left in an unstable state if an exception occurred during ``retention`` or ``compression`` process (`#238 <https://github.com/Delgan/loguru/issues/238>`_).
 
 
 `0.4.1`_ (2020-01-19)

loguru/_file_sink.py

@@ -160,6 +160,7 @@ def __init__(
         self._retention_function = self._make_retention_function(retention)
         self._compression_function = self._make_compression_function(compression)
 
+        self._initialized = False
         self._file = None
         self._file_path = None
 
@@ -168,7 +169,7 @@ def __init__(
 
     def write(self, message):
         if self._file is None:
-            self._initialize_file(rename_existing=False)
+            self._initialize_file(rename_existing=self._initialized)
 
         if self._rotation_function is not None and self._rotation_function(message, self._file):
             self._terminate(teardown=True)
@@ -190,6 +191,7 @@ def _initialize_file(self, *, rename_existing):
 
         self._file_path = new_path
         self._file = open(new_path, **self._kwargs)
+        self._initialized = True
 
     @staticmethod
     def _make_glob_patterns(path):
@@ -335,12 +337,16 @@ def stop(self):
         self._terminate(teardown=self._rotation_function is None)
 
     def _terminate(self, *, teardown):
+        filepath = self._file_path
+
         if self._file is not None:
             self._file.close()
+            self._file = None
+            self._file_path = None
 
         if teardown:
-            if self._compression_function is not None and self._file_path is not None:
-                self._compression_function(self._file_path)
+            if self._compression_function is not None and filepath is not None:
+                self._compression_function(filepath)
 
             if self._retention_function is not None:
                 logs = {
@@ -351,8 +357,5 @@ def _terminate(self, *, teardown):
                 }
                 self._retention_function(list(logs))
 
-        self._file = None
-        self._file_path = None
-
     async def complete(self):
         pass

tests/test_filesink_compression.py

@@ -122,6 +122,99 @@ def test_renaming_compression_dest_exists_with_time(monkeypatch, monkeypatch_dat
     ).check(exists=1)
 
 
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_compression_at_rotation(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_compression(file):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Compression error")
+
+    logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        compression=failing_compression,
+        rotation=0,
+        catch=True,
+        delay=delay,
+    )
+    logger.debug("AAA")
+    logger.debug("BBB")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 3
+    assert [file.read() for file in files] == ["", "", "BBB\n"]
+    assert out == ""
+    assert err.count("Logging error in Loguru Handler") == 1
+    assert err.count("Exception: Compression error") == 1
+
+
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_compression_at_rotation_not_caught(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_compression(file):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Compression error")
+
+    logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        compression=failing_compression,
+        rotation=0,
+        catch=False,
+        delay=delay,
+    )
+    with pytest.raises(Exception, match="Compression error"):
+        logger.debug("AAA")
+    logger.debug("BBB")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 3
+    assert [file.read() for file in files] == ["", "", "BBB\n"]
+    assert out == err == ""
+
+
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_compression_at_remove(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_compression(file):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Compression error")
+
+    i = logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        compression=failing_compression,
+        catch=True,
+        delay=delay,
+    )
+    logger.debug("AAA")
+
+    with pytest.raises(Exception, match=r"Compression error"):
+        logger.remove(i)
+
+    logger.debug("Nope")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 1
+    assert tmpdir.join("test.log").read() == "AAA\n"
+    assert out == err == ""
+
+
 @pytest.mark.parametrize("compression", [0, True, os, object(), {"zip"}])
 def test_invalid_compression(compression):
     with pytest.raises(TypeError):

tests/test_filesink_retention.py

@@ -270,6 +270,102 @@ def test_no_renaming(tmpdir):
     assert tmpdir.join("test.log").read() == "test\n"
 
 
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_retention_at_rotation(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_retention(files):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Retention error")
+
+    logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        retention=failing_retention,
+        rotation=0,
+        catch=True,
+        delay=delay,
+    )
+
+    logger.debug("AAA")
+    logger.debug("BBB")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 3
+    assert [file.read() for file in files] == ["", "", "BBB\n"]
+    assert out == ""
+    assert err.count("Logging error in Loguru Handler") == 1
+    assert err.count("Exception: Retention error") == 1
+
+
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_retention_at_rotation_not_caught(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_retention(files):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Retention error")
+
+    logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        retention=failing_retention,
+        rotation=0,
+        catch=False,
+        delay=delay,
+    )
+
+    with pytest.raises(Exception, match=r"Retention error"):
+        logger.debug("AAA")
+
+    logger.debug("BBB")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 3
+    assert [file.read() for file in files] == ["", "", "BBB\n"]
+    assert out == err == ""
+
+
+@pytest.mark.parametrize("delay", [True, False])
+def test_exception_during_retention_at_remove(tmpdir, capsys, delay):
+    raising = True
+
+    def failing_retention(files):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Retention error")
+
+    i = logger.add(
+        str(tmpdir.join("test.log")),
+        format="{message}",
+        retention=failing_retention,
+        catch=False,
+        delay=delay,
+    )
+    logger.debug("AAA")
+
+    with pytest.raises(Exception, match=r"Retention error"):
+        logger.remove(i)
+
+    logger.debug("Nope")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    assert len(files) == 1
+    assert tmpdir.join("test.log").read() == "AAA\n"
+    assert out == err == ""
+
+
 @pytest.mark.parametrize("retention", [datetime.time(12, 12, 12), os, object()])
 def test_invalid_retention(retention):
     with pytest.raises(TypeError):

tests/test_filesink_rotation.py

@@ -538,6 +538,60 @@ def rotate(file, msg):
     )
 
 
+def test_exception_during_rotation(tmpdir, capsys):
+    raising = True
+
+    def failing_rotation(_, __):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Rotation error")
+        return False
+
+    logger.add(
+        str(tmpdir.join("test.log")), rotation=failing_rotation, format="{message}", catch=True
+    )
+
+    logger.info("A")
+    logger.info("B")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    len(files) == 1
+    assert tmpdir.join("test.log").read() == "B\n"
+    assert out == ""
+    assert err.count("Logging error in Loguru Handler") == 1
+    assert err.count("Exception: Rotation error") == 1
+
+
+def test_exception_during_rotation_not_caught(tmpdir, capsys):
+    raising = True
+
+    def failing_rotation(_, __):
+        nonlocal raising
+        if raising:
+            raising = False
+            raise Exception("Rotation error")
+        return False
+
+    logger.add(
+        str(tmpdir.join("test.log")), rotation=failing_rotation, format="{message}", catch=False
+    )
+
+    with pytest.raises(Exception, match=r"Rotation error"):
+        logger.info("A")
+
+    logger.info("B")
+
+    files = sorted(tmpdir.listdir())
+    out, err = capsys.readouterr()
+
+    len(files) == 1
+    assert tmpdir.join("test.log").read() == "B\n"
+    assert out == err == ""
+
+
 @pytest.mark.parametrize(
     "rotation", [object(), os, datetime.date(2017, 11, 11), datetime.datetime.now(), 1j]
 )