[MemFS] Release memory when close() is called

CHANGELOG.md

@@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [2.4.9] - (Unreleased)
+
+### Changed
+
+- `MemFS` now immediately releases all memory it holds when `close()` is called,
+  rather than when it gets garbage collected. Closes [issue #308](https://github.com/PyFilesystem/pyfilesystem2/issues/).
+
 ## [2.4.8] - 2019-06-12
 
 ### Changed

CONTRIBUTORS.md

@@ -7,3 +7,4 @@ Many thanks to the following developers for contributing to this project:
 - [Martin Larralde](https://github.com/althonos)
 - [Will McGugan](https://github.com/willmcgugan)
 - [Zmej Serow](https://github.com/zmej-serow)
+- [Diego Argueta](https://github.com/dargueta)

fs/memoryfs.py

@@ -207,6 +207,44 @@ def __init__(self, resource_type, name):
         if not self.is_dir:
             self._bytes_file = io.BytesIO()
 
+    def clean_up(self, force=False):
+        # type: (bool) -> None
+        """Release resources held by this directory entry.
+
+        Arguments:
+            force (bool):
+                Force remaining open files to close and delete subdirectory entries.
+
+        Raises:
+            fs.errors.ResourceLocked:
+                This directory entry has remaining open files and ``force`` is `False`.
+            fs.errors.DirectoryNotEmpty:
+                This directory has remaining entries and ``force`` is `False`.
+        """
+        with self.lock:
+            if self._open_files and not force:
+                raise errors.ResourceLocked(
+                    path=self.name,
+                    msg="Can't release resources for %r: %d open file(s) remain(s)."
+                    % (self.name, len(self._open_files))
+                )
+            if self._dir and not force:
+                raise errors.DirectoryNotEmpty(
+                    path=self.name,
+                    msg="Can't release resources for %r: directory isn't empty." % self.name
+                )
+
+            if self._bytes_file is not None:
+                self._bytes_file.close()
+
+            if force:
+                for fdesc in self._open_files:
+                    fdesc.close()
+
+                for entry in six.itervalues(self._dir):
+                    entry.clean_up(force=force)
+                self._dir.clear()
+
     @property
     def bytes_file(self):
         # type: () -> Optional[io.BytesIO]
@@ -254,7 +292,8 @@ def set_entry(self, name, dir_entry):
 
     def remove_entry(self, name):
         # type: (Text) -> None
-        del self._dir[name]
+        dirent = self._dir.pop(name)
+        dirent.clean_up()
 
     def __contains__(self, name):
         # type: (object) -> bool
@@ -340,6 +379,11 @@ def _get_dir_entry(self, dir_path):
                 current_entry = current_entry.get_entry(path_component)
             return current_entry
 
+    def close(self):
+        # type: () -> None
+        self.root.clean_up(force=True)
+        return super(MemoryFS, self).close()
+
     def getinfo(self, path, namespaces=None):
         # type: (Text, Optional[Collection[Text]]) -> Info
         namespaces = namespaces or ()

tests/test_memoryfs.py

@@ -1,13 +1,93 @@
 from __future__ import unicode_literals
 
+import posixpath
 import unittest
 
+from fs import errors
 from fs import memoryfs
 from fs.test import FSTestCases
+from fs.test import UNICODE_TEXT
+
+try:
+    # Only supported on Python 3.4+
+    import tracemalloc
+except ImportError:
+    tracemalloc = None
 
 
 class TestMemoryFS(FSTestCases, unittest.TestCase):
     """Test OSFS implementation."""
 
     def make_fs(self):
         return memoryfs.MemoryFS()
+
+    def _create_many_files(self):
+        for parent_dir in {"/", "/one", "/one/two", "/one/other-two/three"}:
+            self.fs.makedirs(parent_dir, recreate=True)
+            for file_id in range(50):
+                self.fs.writetext(
+                    posixpath.join(parent_dir, str(file_id)), UNICODE_TEXT
+                )
+
+    @unittest.skipIf(
+        not tracemalloc, "`tracemalloc` isn't supported on this Python version."
+    )
+    def test_close_mem_free(self):
+        """Ensure all file memory is freed when calling close().
+
+        Prevents regression against issue #308.
+        """
+        trace_filters = [tracemalloc.Filter(True, "*/memoryfs.py")]
+        tracemalloc.start()
+
+        before = tracemalloc.take_snapshot().filter_traces(trace_filters)
+        self._create_many_files()
+        after_create = tracemalloc.take_snapshot().filter_traces(trace_filters)
+
+        self.fs.close()
+        after_close = tracemalloc.take_snapshot().filter_traces(trace_filters)
+        tracemalloc.stop()
+
+        [diff_create] = after_create.compare_to(
+            before, key_type="filename", cumulative=True
+        )
+        self.assertGreater(
+            diff_create.size_diff,
+            0,
+            "Memory usage didn't increase after creating files; diff is %0.2f KiB."
+            % (diff_create.size_diff / 1024.0),
+        )
+
+        [diff_close] = after_close.compare_to(
+            after_create, key_type="filename", cumulative=True
+        )
+        self.assertLess(
+            diff_close.size_diff,
+            0,
+            "Memory usage increased after closing the file system; diff is %0.2f KiB."
+            % (diff_close.size_diff / 1024.0),
+        )
+
+    def test_dirent_clean_up__open_file_crashes(self):
+        """clean_up() crashes if files are open and force=False"""
+        self._create_many_files()
+
+        for path in {"/one/other-two/three/0", "/one/two/2", "/2"}:
+            fdesc = self.fs.open(path, "r")
+
+            with self.subTest(to_drop=path):
+                dirent = self.fs._get_dir_entry(path)
+                self.assertIsNotNone(dirent, "Couldn't find %s" % path)
+                with self.assertRaises(errors.ResourceLocked):
+                    dirent.clean_up()
+
+    def test_dirent_clean_up__subentries_crashes(self):
+        """clean_up() crashes if the dirent has entries and force=False"""
+        self._create_many_files()
+
+        for path in {"/one/other-two/three", "/one/two"}:
+            with self.subTest(to_drop=path):
+                dirent = self.fs._get_dir_entry(path)
+                self.assertIsNotNone(dirent, "Couldn't find %s" % path)
+                with self.assertRaises(errors.DirectoryNotEmpty):
+                    dirent.clean_up()