Call atexit handlers before test tearDown to remove errors on Windows (#6667)

When certbot is executing, several resources are opened. It is typically file handles and locks on them. Of course, theses resources need to be cleanup. It is done in Certbot by registering cleanup functions through atexit module, that ensures theses functions will be called when Certbot exit. This allow to not care about resource cleanup everywhere in the code, as it is processed globally.

The problem with atexit is it cleanup functions are called when the Python program exit. If the program is Certbot itself when used, this is Pytest in unit test execution. So during a unit test execution, cleanup is not called after a test and before its tearDown, but when Pytest exit, so way after tests and their respective tearDown.

But many tearDown implies to delete folders where this kind of resources are hold.

This is never a problem on Linux, thanks to its non-blocking file handling. It is usually not a problem on Windows, despite its blocking approach. But if the tearDown requires folder cleanup, exceptions are raised, and currently hidden as warnings. There is currently 504 exceptions of this type in Certbot core tests on Windows.

This PR starts to correct this situation. To do so, some of the functions cleanup normally called through atexit, are explicitly called as part of the tearDown process of relevant test classes, before directory removal is done. Theses situations come all from the certbot.tests.util.TempDirTestCase, so the code is in this specific tearDown process.

As a consequence, exceptions drop from 504 to 64.

Then there are still a significant part of them, that will be handled in later mitigation.

* Call atexit handlers before test tearDown to reduce errors on Windows

* Clear locks dict after global releasing execution

* Remove last tearDown errors.

* Clean out mock on open.

* Remove a test

* Reenable some tests

certbot/tests/lock_test.py

@@ -10,7 +10,6 @@ from certbot import errors
 from certbot.tests import util as test_util
 
 
-@test_util.broken_on_windows
 class LockDirTest(test_util.TempDirTestCase):
     """Tests for certbot.lock.lock_dir."""
     @classmethod
@@ -25,7 +24,6 @@ class LockDirTest(test_util.TempDirTestCase):
         test_util.lock_and_call(assert_raises, lock_path)
 
 
-@test_util.broken_on_windows
 class LockFileTest(test_util.TempDirTestCase):
     """Tests for certbot.lock.LockFile."""
     @classmethod
@@ -37,6 +35,7 @@ class LockFileTest(test_util.TempDirTestCase):
         super(LockFileTest, self).setUp()
         self.lock_path = os.path.join(self.tempdir, 'test.lock')
 
+    @test_util.broken_on_windows
     def test_acquire_without_deletion(self):
         # acquire the lock in another process but don't delete the file
         child = multiprocessing.Process(target=self._call,
@@ -54,6 +53,7 @@ class LockFileTest(test_util.TempDirTestCase):
             self.assertRaises, errors.LockError, self._call, self.lock_path)
         test_util.lock_and_call(assert_raises, self.lock_path)
 
+    @test_util.broken_on_windows
     def test_locked_repr(self):
         lock_file = self._call(self.lock_path)
         locked_repr = repr(lock_file)
@@ -71,6 +71,7 @@ class LockFileTest(test_util.TempDirTestCase):
         self.assertTrue(lock_file.__class__.__name__ in lock_repr)
         self.assertTrue(self.lock_path in lock_repr)
 
+    @test_util.broken_on_windows
     def test_race(self):
         should_delete = [True, False]
         stat = os.stat
@@ -86,11 +87,13 @@ class LockFileTest(test_util.TempDirTestCase):
             self._call(self.lock_path)
         self.assertFalse(should_delete)
 
+    @test_util.broken_on_windows
     def test_removed(self):
         lock_file = self._call(self.lock_path)
         lock_file.release()
         self.assertFalse(os.path.exists(self.lock_path))
 
+    @test_util.broken_on_windows
     @mock.patch('certbot.compat.fcntl.lockf')
     def test_unexpected_lockf_err(self, mock_lockf):
         msg = 'hi there'

certbot/tests/util.py

@@ -3,14 +3,15 @@
 .. warning:: This module is not part of the public API.
 
 """
+import logging
 import multiprocessing
 import os
 import pkg_resources
 import shutil
+import stat
 import tempfile
 import unittest
 import sys
-import warnings
 
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives import serialization
@@ -328,22 +329,22 @@ class TempDirTestCase(unittest.TestCase):
 
     def tearDown(self):
         """Execute after test"""
-        # On Windows we have various files which are not correctly closed at the time of tearDown.
-        # For know, we log them until a proper file close handling is written.
-        # Useful for development only, so no warning when we are on a CI process.
-        def onerror_handler(_, path, excinfo):
-            """On error handler"""
-            if not os.environ.get('APPVEYOR'): # pragma: no cover
-                message = ('Following error occurred when deleting the tempdir {0}'
-                           ' for path {1} during tearDown process: {2}'
-                           .format(self.tempdir, path, str(excinfo)))
-                warnings.warn(message)
-        shutil.rmtree(self.tempdir, onerror=onerror_handler)
+        # Cleanup opened resources after a test. This is usually done through atexit handlers in
+        # Certbot, but during tests, atexit will not run registered functions before tearDown is
+        # called and instead will run them right before the entire test process exits.
+        # It is a problem on Windows, that does not accept to clean resources before closing them.
+        logging.shutdown()
+        util._release_locks()  # pylint: disable=protected-access
+
+        def handle_rw_files(_, path, __):
+            """Handle read-only files, that will fail to be removed on Windows."""
+            os.chmod(path, stat.S_IWRITE)
+            os.remove(path)
+        shutil.rmtree(self.tempdir, onerror=handle_rw_files)
+
 
 class ConfigTestCase(TempDirTestCase):
-    """Test class which sets up a NamespaceConfig object.
-
-    """
+    """Test class which sets up a NamespaceConfig object."""
     def setUp(self):
         super(ConfigTestCase, self).setUp()
         self.config = configuration.NamespaceConfig(

certbot/tests/util_test.py

@@ -191,7 +191,12 @@ class CheckPermissionsTest(test_util.TempDirTestCase):
 
     def test_wrong_mode(self):
         os.chmod(self.tempdir, 0o400)
-        self.assertFalse(self._call(0o600))
+        try:
+            self.assertFalse(self._call(0o600))
+        finally:
+            # Without proper write permissions, Windows is unable to delete a folder,
+            # even with admin permissions. Write access must be explicitly set first.
+            os.chmod(self.tempdir, 0o700)
 
 
 class UniqueFileTest(test_util.TempDirTestCase):
@@ -277,20 +282,9 @@ class UniqueLineageNameTest(test_util.TempDirTestCase):
         for f, _ in items:
             f.close()
 
-    @mock.patch("certbot.util.os.fdopen")
-    def test_failure(self, mock_fdopen):
-        err = OSError("whoops")
-        err.errno = errno.EIO
-        mock_fdopen.side_effect = err
-        self.assertRaises(OSError, self._call, "wow")
-
-    @mock.patch("certbot.util.os.fdopen")
-    def test_subsequent_failure(self, mock_fdopen):
-        self._call("wow")
-        err = OSError("whoops")
-        err.errno = errno.EIO
-        mock_fdopen.side_effect = err
-        self.assertRaises(OSError, self._call, "wow")
+    def test_failure(self):
+        with mock.patch("certbot.util.os.open", side_effect=OSError(errno.EIO)):
+            self.assertRaises(OSError, self._call, "wow")
 
 
 class SafelyRemoveTest(test_util.TempDirTestCase):

certbot/util.py

@@ -142,6 +142,7 @@ def _release_locks():
         except:  # pylint: disable=bare-except
             msg = 'Exception occurred releasing lock: {0!r}'.format(dir_lock)
             logger.debug(msg, exc_info=True)
+    _LOCKS.clear()
 
 
 def set_up_core_dir(directory, mode, uid, strict):
@@ -225,9 +226,8 @@ def safe_open(path, mode="w", chmod=None, buffering=None):
     fdopen_args = ()  # type: Union[Tuple[()], Tuple[int]]
     if buffering is not None:
         fdopen_args = (buffering,)
-    return os.fdopen(
-        os.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args),
-        mode, *fdopen_args)
+    fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args)
+    return os.fdopen(fd, mode, *fdopen_args)
 
 
 def _unique_file(path, filename_pat, count, chmod, mode):