[Windows] Fix closing files descriptors during unit tests (#7326)

* Fix file descriptor cleanup during tests on Windows

* Fix lint

* Remove useless tearDown

* Clean pylint

certbot-nginx/certbot_nginx/tests/util.py

@@ -3,7 +3,6 @@ import copy
 import shutil
 import tempfile
 import unittest
-import warnings
 
 import josepy as jose
 import mock
@@ -11,6 +10,7 @@ import pkg_resources
 import zope.component
 
 from certbot import configuration
+from certbot import util
 from certbot.compat import os
 from certbot.plugins import common
 from certbot.tests import util as test_util
@@ -34,20 +34,16 @@ class NginxTest(unittest.TestCase):  # pylint: disable=too-few-public-methods
             "rsa512_key.pem"))
 
     def tearDown(self):
-        # 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 path {0}'
-                           'during tearDown process: {1}'.format(path, str(excinfo)))
-                warnings.warn(message)
+        # 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.
+        util._release_locks()  # pylint: disable=protected-access
 
-        shutil.rmtree(self.temp_dir, onerror=onerror_handler)
-        shutil.rmtree(self.config_dir, onerror=onerror_handler)
-        shutil.rmtree(self.work_dir, onerror=onerror_handler)
-        shutil.rmtree(self.logs_dir, onerror=onerror_handler)
+        shutil.rmtree(self.temp_dir)
+        shutil.rmtree(self.config_dir)
+        shutil.rmtree(self.work_dir)
+        shutil.rmtree(self.logs_dir)
 
 
 def get_data_filename(filename):

certbot/compat/filesystem.py

@@ -166,11 +166,11 @@ def open(file_path, flags, mode=0o777):  # pylint: disable=redefined-builtin
         # See https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptordacl  # pylint: disable=line-too-long
         security.SetSecurityDescriptorDacl(1, dacl, 0)
 
+        handle = None
         try:
             handle = win32file.CreateFile(file_path, win32file.GENERIC_READ,
                                           win32file.FILE_SHARE_READ & win32file.FILE_SHARE_WRITE,
                                           attributes, disposition, 0, None)
-            handle.Close()
         except pywintypes.error as err:
             # Handle native windows errors into python errors to be consistent with the API
             # of os.open in the situation of a file already existing or locked.
@@ -179,6 +179,9 @@ def open(file_path, flags, mode=0o777):  # pylint: disable=redefined-builtin
             if err.winerror == winerror.ERROR_SHARING_VIOLATION:
                 raise OSError(errno.EACCES, err.strerror)
             raise err
+        finally:
+            if handle:
+                handle.Close()
 
         # At this point, the file that did not exist has been created with proper permissions,
         # so os.O_CREAT and os.O_EXCL are not needed anymore. We remove them from the flags to

certbot/tests/compat/filesystem_test.py

@@ -210,15 +210,15 @@ class WindowsOpenTest(TempDirTestCase):
     def _test_one_creation(self, num, file_exist, flags):
         one_file = os.path.join(self.tempdir, str(num))
         if file_exist and not os.path.exists(one_file):
-            open(one_file, 'w').close()
+            with open(one_file, 'w'):
+                pass
 
         handler = None
         try:
             handler = filesystem.open(one_file, flags)
-        except BaseException as err:
+        finally:
             if handler:
                 os.close(handler)
-            raise err
 
 
 @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')

certbot/tests/util.py

@@ -5,7 +5,6 @@
 """
 import logging
 import shutil
-import stat
 import sys
 import tempfile
 import unittest
@@ -339,16 +338,7 @@ class TempDirTestCase(unittest.TestCase):
         logging.getLogger().handlers = []
         util._release_locks()  # pylint: disable=protected-access
 
-        def handle_rw_files(_, path, __):
-            """Handle read-only files, that will fail to be removed on Windows."""
-            filesystem.chmod(path, stat.S_IWRITE)
-            try:
-                os.remove(path)
-            except (IOError, OSError):
-                # TODO: remote the try/except once all logic from windows file permissions is merged
-                if os.name != 'nt':
-                    raise
-        shutil.rmtree(self.tempdir, onerror=handle_rw_files)
+        shutil.rmtree(self.tempdir)
 
 
 class ConfigTestCase(TempDirTestCase):