Merge pull request #475 from PyFilesystem/fix-scandir

Fix minor issues in `OSFS.scandir`

Author: althonos

CHANGELOG.md

@@ -40,6 +40,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `WrapCachedDir.isdir` and `WrapCachedDir.isfile` raising a `ResourceNotFound` error on non-existing path ([#470](https://github.com/PyFilesystem/pyfilesystem2/pull/470)).
 - `FTPFS` not listing certain entries with sticky/SUID/SGID permissions set by Linux server ([#473](https://github.com/PyFilesystem/pyfilesystem2/pull/473)).
   Closes [#451](https://github.com/PyFilesystem/pyfilesystem2/issues/451).
+- `scandir` iterator not being closed explicitly in `OSFS.scandir`, occasionally causing a `ResourceWarning` 
+  to be thrown. Closes [#311](https://github.com/PyFilesystem/pyfilesystem2/issues/311).
 
 
 ## [2.4.12] - 2021-01-14

fs/osfs.py

@@ -475,6 +475,7 @@ def _scandir(self, path, namespaces=None):
             # type: (Text, Optional[Collection[Text]]) -> Iterator[Info]
             self.check()
             namespaces = namespaces or ()
+            requires_stat = not {"details", "stat", "access"}.isdisjoint(namespaces)
             _path = self.validatepath(path)
             if _WINDOWS_PLATFORM:
                 sys_path = os.path.join(
@@ -483,39 +484,47 @@ def _scandir(self, path, namespaces=None):
             else:
                 sys_path = self._to_sys_path(_path)  # type: ignore
             with convert_os_errors("scandir", path, directory=True):
-                for dir_entry in scandir(sys_path):
-                    info = {
-                        "basic": {
-                            "name": fsdecode(dir_entry.name),
-                            "is_dir": dir_entry.is_dir(),
-                        }
-                    }
-                    if "details" in namespaces:
-                        stat_result = dir_entry.stat()
-                        info["details"] = self._make_details_from_stat(stat_result)
-                    if "stat" in namespaces:
-                        stat_result = dir_entry.stat()
-                        info["stat"] = {
-                            k: getattr(stat_result, k)
-                            for k in dir(stat_result)
-                            if k.startswith("st_")
-                        }
-                    if "lstat" in namespaces:
-                        lstat_result = dir_entry.stat(follow_symlinks=False)
-                        info["lstat"] = {
-                            k: getattr(lstat_result, k)
-                            for k in dir(lstat_result)
-                            if k.startswith("st_")
+                scandir_iter = scandir(sys_path)
+                try:
+                    for dir_entry in scandir_iter:
+                        info = {
+                            "basic": {
+                                "name": fsdecode(dir_entry.name),
+                                "is_dir": dir_entry.is_dir(),
+                            }
                         }
-                    if "link" in namespaces:
-                        info["link"] = self._make_link_info(
-                            os.path.join(sys_path, dir_entry.name)
-                        )
-                    if "access" in namespaces:
-                        stat_result = dir_entry.stat()
-                        info["access"] = self._make_access_from_stat(stat_result)
-
-                    yield Info(info)
+                        if requires_stat:
+                            stat_result = dir_entry.stat()
+                            if "details" in namespaces:
+                                info["details"] = self._make_details_from_stat(
+                                    stat_result
+                                )
+                            if "stat" in namespaces:
+                                info["stat"] = {
+                                    k: getattr(stat_result, k)
+                                    for k in dir(stat_result)
+                                    if k.startswith("st_")
+                                }
+                            if "access" in namespaces:
+                                info["access"] = self._make_access_from_stat(
+                                    stat_result
+                                )
+                        if "lstat" in namespaces:
+                            lstat_result = dir_entry.stat(follow_symlinks=False)
+                            info["lstat"] = {
+                                k: getattr(lstat_result, k)
+                                for k in dir(lstat_result)
+                                if k.startswith("st_")
+                            }
+                        if "link" in namespaces:
+                            info["link"] = self._make_link_info(
+                                os.path.join(sys_path, dir_entry.name)
+                            )
+
+                        yield Info(info)
+                finally:
+                    if sys.version_info >= (3, 6):
+                        scandir_iter.close()
 
     else:
 

fs/test.py

@@ -16,6 +16,7 @@
 import os
 import time
 import unittest
+import warnings
 
 import fs.copy
 import fs.move
@@ -884,8 +885,9 @@ def test_open_files(self):
             self.assertFalse(f.closed)
         self.assertTrue(f.closed)
 
-        iter_lines = iter(self.fs.open("text"))
-        self.assertEqual(next(iter_lines), "Hello\n")
+        with self.fs.open("text") as f:
+            iter_lines = iter(f)
+            self.assertEqual(next(iter_lines), "Hello\n")
 
         with self.fs.open("unicode", "w") as f:
             self.assertEqual(12, f.write("Héllo\nWörld\n"))
@@ -1594,8 +1596,10 @@ def test_files(self):
         self.assert_bytes("foo2", b"help")
 
         # Test __del__ doesn't throw traceback
-        f = self.fs.open("foo2", "r")
-        del f
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore")
+            f = self.fs.open("foo2", "r")
+            del f
 
         with self.assertRaises(IOError):
             with self.fs.open("foo2", "r") as f:

tests/test_osfs.py

@@ -8,6 +8,7 @@
 import tempfile
 import sys
 import unittest
+import warnings
 
 from fs import osfs, open_fs
 from fs.path import relpath, dirname
@@ -25,6 +26,14 @@
 class TestOSFS(FSTestCases, unittest.TestCase):
     """Test OSFS implementation."""
 
+    @classmethod
+    def setUpClass(cls):
+        warnings.simplefilter("error")
+
+    @classmethod
+    def tearDownClass(cls):
+        warnings.simplefilter(warnings.defaultaction)
+
     def make_fs(self):
         temp_dir = tempfile.mkdtemp("fstestosfs")
         return osfs.OSFS(temp_dir)