From 0e574aa5760f8511abba6ccaf805f34c2caeb996 Mon Sep 17 00:00:00 2001
From: Rob Browning <rlb@defaultvalue.org>
Date: Sun, 24 Oct 2021 12:24:16 -0500
Subject: [PATCH] Check that all context managed objects are properly closed

Ensure all of our context managed objects have a __del__ that asserts
that the instance has been properly closed so that we'll be more
likely to notice related oversights.

This will only work in cases where __del__ is called before shutdown,
but that should normally be the case for cpython.

Signed-off-by: Rob Browning <rlb@defaultvalue.org>
Tested-by: Rob Browning <rlb@defaultvalue.org>
---
 lib/bup/bloom.py     |  5 +++++
 lib/bup/client.py    | 25 ++++++++++++++++++-------
 lib/bup/cmd/split.py |  4 +++-
 lib/bup/compat.py    | 19 +++++++++++++++++++
 lib/bup/git.py       | 16 ++++++++++++++++
 lib/bup/helpers.py   |  5 +++++
 lib/bup/hlinkdb.py   |  6 ++++++
 lib/bup/index.py     | 21 +++++++++++++++++++++
 lib/bup/midx.py      |  5 +++++
 lib/bup/repo.py      | 13 +++++++++++--
 lib/bup/vfs.py       |  7 ++++++-
 11 files changed, 115 insertions(+), 11 deletions(-)

diff --git a/lib/bup/bloom.py b/lib/bup/bloom.py
index 97024b9..b686127 100644
--- a/lib/bup/bloom.py
+++ b/lib/bup/bloom.py
@@ -107,6 +107,7 @@ bloom_add = _helpers.bloom_add
 class ShaBloom:
     """Wrapper which contains data from multiple index files. """
     def __init__(self, filename, f=None, readwrite=False, expected=-1):
+        self.closed = False
         self.name = filename
         self.readwrite = readwrite
         self.file = None
@@ -183,6 +184,7 @@ class ShaBloom:
         return self.map and self.bits
 
     def close(self):
+        self.closed = True
         try:
             if self.map and self.readwrite:
                 debug2("bloom: closing with %d entries\n" % self.entries)
@@ -198,6 +200,9 @@ class ShaBloom:
         finally:  # This won't handle pending exceptions correctly in py2
             self._init_failed()
 
+    def __del__(self):
+        assert self.closed
+
     def __enter__(self):
         return self
 
diff --git a/lib/bup/client.py b/lib/bup/client.py
index ed902b7..7c284c4 100644
--- a/lib/bup/client.py
+++ b/lib/bup/client.py
@@ -69,6 +69,7 @@ def parse_remote(remote):
 
 class Client:
     def __init__(self, remote, create=False):
+        self.closed = False
         self._busy = self.conn = None
         self.sock = self.p = self.pout = self.pin = None
         is_reverse = environ.get(b'BUP_SERVER_REVERSE')
@@ -117,14 +118,8 @@ class Client:
             self.check_ok()
         self.sync_indexes()
 
-    def __enter__(self):
-        return self
-
-    def __exit__(self, type, value, traceback):
-        with pending_raise(value, rethrow=False):
-            self.close()
-
     def close(self):
+        self.closed = True
         if self.conn and not self._busy:
             self.conn.write(b'quit\n')
         if self.pin:
@@ -146,6 +141,16 @@ class Client:
         self.conn = None
         self.sock = self.p = self.pin = self.pout = None
 
+    def __del__(self):
+        assert self.closed
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, type, value, traceback):
+        with pending_raise(value, rethrow=False):
+            self.close()
+
     def check_ok(self):
         if self.p:
             rv = self.p.poll()
@@ -494,6 +499,7 @@ class PackWriter_Remote(git.PackWriter):
                                 compression_level=compression_level,
                                 max_pack_size=max_pack_size,
                                 max_pack_objects=max_pack_objects)
+        self.remote_closed = False
         self.file = conn
         self.filename = b'remote socket'
         self.suggest_packs = suggest_packs
@@ -528,10 +534,15 @@ class PackWriter_Remote(git.PackWriter):
 
     def close(self):
         # Called by inherited __exit__
+        self.remote_closed = True
         id = self._end()
         self.file = None
         return id
 
+    def __del__(self):
+        assert self.remote_closed
+        super(PackWriter_Remote, self).__del__()
+
     def abort(self):
         raise ClientError("don't know how to abort remote pack writing")
 
diff --git a/lib/bup/cmd/split.py b/lib/bup/cmd/split.py
index 59ad46c..87da1c2 100755
--- a/lib/bup/cmd/split.py
+++ b/lib/bup/cmd/split.py
@@ -50,7 +50,9 @@ class NoOpPackWriter:
     def __exit__(self, type, value, traceback):
         return None  # since close() does nothing
     def close(self):
-        return None
+        assert self.closed
+    def __del__(self):
+        assert self.closed
     def new_blob(self, content):
         return git.calc_hash(b'blob', content)
     def new_tree(self, shalist):
diff --git a/lib/bup/compat.py b/lib/bup/compat.py
index 68006f5..dfeb51a 100644
--- a/lib/bup/compat.py
+++ b/lib/bup/compat.py
@@ -48,13 +48,17 @@ if py3:
 
         """
         def __init__(self, ex, rethrow=True):
+            self.closed = False
             self.ex = ex
             self.rethrow = rethrow
         def __enter__(self):
             return None
         def __exit__(self, exc_type, exc_value, traceback):
+            self.closed = True
             if not exc_type and self.ex and self.rethrow:
                 raise self.ex
+        def __del__(self):
+            assert self.closed
 
     def items(x):
         return x.items()
@@ -146,12 +150,14 @@ else:  # Python 2
 
         """
         def __init__(self, ex, rethrow=True):
+            self.closed = False
             self.ex = ex
             self.rethrow = rethrow
         def __enter__(self):
             if self.ex:
                 add_ex_tb(self.ex)
         def __exit__(self, exc_type, exc_value, traceback):
+            self.closed = True
             if exc_value:
                 if self.ex:
                     add_ex_tb(exc_value)
@@ -159,6 +165,8 @@ else:  # Python 2
                 return
             if self.rethrow and self.ex:
                 raise self.ex
+        def __del__(self):
+            assert self.closed
 
     def dump_traceback(ex):
         stack = [ex]
@@ -217,15 +225,26 @@ else:  # Python 2
 
     buffer = buffer
 
+    assert not hasattr(py_mmap.mmap, '__del__')
     assert not hasattr(py_mmap.mmap, '__enter__')
     assert not hasattr(py_mmap.mmap, '__exit__')
 
     class mmap(py_mmap.mmap):
+        def __init__(self, *args, **kwargs):
+            self._bup_closed = False
+            # Silence deprecation warnings.  mmap's current parent is
+            # object, which accepts no params and as of at least 2.7
+            # warns about them.
+            if py_mmap.mmap.__init__ is not object.__init__:
+                super(mmap, self).__init__(self, *args, **kwargs)
         def __enter__(self):
             return self
         def __exit__(self, type, value, traceback):
+            self._bup_closed = True
             with pending_raise(value, rethrow=False):
                 self.close()
+        def __del__(self):
+            assert self._bup_closed
 
 try:
     import bup_main
diff --git a/lib/bup/git.py b/lib/bup/git.py
index e3b235c..c038fd1 100644
--- a/lib/bup/git.py
+++ b/lib/bup/git.py
@@ -414,6 +414,7 @@ class PackIdx:
 class PackIdxV1(PackIdx):
     """Object representation of a Git pack index (version 1) file."""
     def __init__(self, filename, f):
+        self.closed = False
         self.name = filename
         self.idxnames = [self.name]
         self.map = mmap_read(f)
@@ -453,15 +454,20 @@ class PackIdxV1(PackIdx):
             yield self.map[ofs : ofs + 20]
 
     def close(self):
+        self.closed = True
         if self.map is not None:
             self.shatable = None
             self.map.close()
             self.map = None
 
+    def __del__(self):
+        assert self.closed
+
 
 class PackIdxV2(PackIdx):
     """Object representation of a Git pack index (version 2) file."""
     def __init__(self, filename, f):
+        self.closed = False
         self.name = filename
         self.idxnames = [self.name]
         self.map = mmap_read(f)
@@ -509,11 +515,15 @@ class PackIdxV2(PackIdx):
             yield self.map[ofs : ofs + 20]
 
     def close(self):
+        self.closed = True
         if self.map is not None:
             self.shatable = None
             self.map.close()
             self.map = None
 
+    def __del__(self):
+        assert self.closed
+
 
 _mpi_count = 0
 class PackIdxList:
@@ -758,6 +768,7 @@ class PackWriter:
     def __init__(self, objcache_maker=_make_objcache, compression_level=1,
                  run_midx=True, on_pack_finish=None,
                  max_pack_size=None, max_pack_objects=None, repo_dir=None):
+        self.closed = False
         self.repo_dir = repo_dir or repo()
         self.file = None
         self.parentfd = None
@@ -949,6 +960,7 @@ class PackWriter:
 
     def abort(self):
         """Remove the pack file from disk."""
+        self.closed = True
         self._end(abort=True)
 
     def breakpoint(self):
@@ -959,8 +971,12 @@ class PackWriter:
 
     def close(self, run_midx=True):
         """Close the pack file and move it to its definitive path."""
+        self.closed = True
         return self._end(run_midx=run_midx)
 
+    def __del__(self):
+        assert self.closed
+
 
 class PackIdxV2Writer:
     def __init__(self):
diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index 0530b51..fdc683b 100644
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -454,11 +454,16 @@ class NotOk(Exception):
 
 class BaseConn:
     def __init__(self, outp):
+        self._base_closed = False
         self.outp = outp
 
     def close(self):
+        self._base_closed = True
         while self._read(65536): pass
 
+    def __del__(self):
+        assert self._base_closed
+
     def _read(self, size):
         raise NotImplementedError("Subclasses must implement _read")
 
diff --git a/lib/bup/hlinkdb.py b/lib/bup/hlinkdb.py
index 05a52ef..29d575e 100644
--- a/lib/bup/hlinkdb.py
+++ b/lib/bup/hlinkdb.py
@@ -19,6 +19,7 @@ class Error(Exception):
 
 class HLinkDB:
     def __init__(self, filename):
+        self.closed = False
         # Map a "dev:ino" node to a list of paths associated with that node.
         self._node_paths = {}
         # Map a path to a "dev:ino" node.
@@ -72,6 +73,7 @@ class HLinkDB:
         self._save_prepared = True
 
     def commit_save(self):
+        self.closed = True
         if not self._save_prepared:
             raise Error('cannot commit save of %r; no save prepared'
                         % self._filename)
@@ -89,6 +91,7 @@ class HLinkDB:
         self._save_prepared = None
 
     def abort_save(self):
+        self.closed = True
         if self._tmpname:
             os.unlink(self._tmpname)
             self._tmpname = None
@@ -100,6 +103,9 @@ class HLinkDB:
         with pending_raise(value, rethrow=True):
             self.abort_save()
 
+    def __del__(self):
+        assert self.closed
+
     def add_path(self, path, dev, ino):
         # Assume path is new.
         node = b'%d:%d' % (dev, ino)
diff --git a/lib/bup/index.py b/lib/bup/index.py
index b9a4013..591079a 100644
--- a/lib/bup/index.py
+++ b/lib/bup/index.py
@@ -50,14 +50,19 @@ class Error(Exception):
 
 class MetaStoreReader:
     def __init__(self, filename):
+        self._closed = False
         self._file = None
         self._file = open(filename, 'rb')
 
     def close(self):
+        self._closed = True
         if self._file:
             self._file.close()
             self._file = None
 
+    def __del__(self):
+        assert self._closed
+
     def __enter__(self):
         return self
 
@@ -75,6 +80,7 @@ class MetaStoreWriter:
     # truncation or corruption somewhat sensibly.
 
     def __init__(self, filename):
+        self._closed = False
         # Map metadata hashes to bupindex.meta offsets.
         self._offsets = {}
         self._filename = filename
@@ -101,10 +107,14 @@ class MetaStoreWriter:
         self._file = open(filename, 'ab')
 
     def close(self):
+        self._closed = True
         if self._file:
             self._file.close()
             self._file = None
 
+    def __del__(self):
+        assert self._closed
+
     def __enter__(self):
         return self
 
@@ -413,6 +423,7 @@ class ExistingEntry(Entry):
 
 class Reader:
     def __init__(self, filename):
+        self.closed = False
         self.filename = filename
         self.m = b''
         self.writable = False
@@ -488,12 +499,16 @@ class Reader:
             self.m.flush()
 
     def close(self):
+        self.closed = True
         self.save()
         if self.writable and self.m:
             self.m.close()
             self.m = None
             self.writable = False
 
+    def __del__(self):
+        assert self.closed
+
     def filter(self, prefixes, wantrecurse=None):
         for (rp, path) in reduce_paths(prefixes):
             any_entries = False
@@ -524,6 +539,7 @@ def pathsplit(p):
 
 class Writer:
     def __init__(self, filename, metastore, tmax):
+        self.closed = False
         self.rootlevel = self.level = Level([], None)
         self.f = None
         self.count = 0
@@ -545,6 +561,7 @@ class Writer:
             self.abort()
 
     def abort(self):
+        self.closed = True
         f = self.f
         self.f = None
         if f:
@@ -563,6 +580,7 @@ class Writer:
         assert(self.level == None)
 
     def close(self):
+        self.closed = True
         self.flush()
         f = self.f
         self.f = None
@@ -570,6 +588,9 @@ class Writer:
             f.close()
             os.rename(self.tmpname, self.filename)
 
+    def __del__(self):
+        assert self.closed
+
     def _add(self, ename, entry):
         if self.lastfile and self.lastfile <= ename:
             raise Error('%r must come before %r'
diff --git a/lib/bup/midx.py b/lib/bup/midx.py
index e245816..1b3708e 100644
--- a/lib/bup/midx.py
+++ b/lib/bup/midx.py
@@ -22,6 +22,7 @@ class PackMidx:
     amounts of files.
     """
     def __init__(self, filename):
+        self.closed = False
         self.name = filename
         self.force_keep = False
         self.map = None
@@ -92,11 +93,15 @@ class PackMidx:
         return self.idxnames[self._get_idx_i(i)]
 
     def close(self):
+        self.closed = True
         if self.map is not None:
             self.fanout = self.shatable = self.whichlist = self.idxnames = None
             self.map.close()
             self.map = None
 
+    def __del__(self):
+        assert self.closed
+
     def exists(self, hash, want_source=False):
         """Return nonempty if the object exists in the index files."""
         global _total_searches, _total_steps
diff --git a/lib/bup/repo.py b/lib/bup/repo.py
index 64feeb7..87305fb 100644
--- a/lib/bup/repo.py
+++ b/lib/bup/repo.py
@@ -21,6 +21,7 @@ def _repo_id(key):
 
 class LocalRepo:
     def __init__(self, repo_dir=None):
+        self.closed = False
         self.repo_dir = realpath(repo_dir or git.repo())
         self._cp = git.cp(self.repo_dir)
         self.update_ref = partial(git.update_ref, repo_dir=self.repo_dir)
@@ -28,7 +29,10 @@ class LocalRepo:
         self._id = _repo_id(self.repo_dir)
 
     def close(self):
-        pass
+        self.closed = True
+
+    def __del__(self):
+        assert self.closed
 
     def __enter__(self):
         return self
@@ -86,6 +90,7 @@ class LocalRepo:
 
 class RemoteRepo:
     def __init__(self, address):
+        self.closed = False
         self.address = address
         self.client = client.Client(address)
         self.new_packwriter = self.client.new_packwriter
@@ -94,10 +99,14 @@ class RemoteRepo:
         self._id = _repo_id(self.address)
 
     def close(self):
-        if self.client:
+        if not self.closed:
+            self.closed = True
             self.client.close()
             self.client = None
 
+    def __del__(self):
+        assert self.closed
+
     def __enter__(self):
         return self
 
diff --git a/lib/bup/vfs.py b/lib/bup/vfs.py
index f5badf6..683c9ac 100644
--- a/lib/bup/vfs.py
+++ b/lib/bup/vfs.py
@@ -195,6 +195,7 @@ class _ChunkReader:
 class _FileReader(object):
     def __init__(self, repo, oid, known_size=None):
         assert len(oid) == 20
+        self.closed = False
         self.oid = oid
         self.ofs = 0
         self.reader = None
@@ -231,10 +232,14 @@ class _FileReader(object):
         return buf
 
     def close(self):
-        pass
+        self.closed = True
+
+    def __del__(self):
+        assert self.closed
 
     def __enter__(self):
         return self
+
     def __exit__(self, type, value, traceback):
         with pending_raise(value, rethrow=False):
             self.close()
-- 
2.39.2