Add commit_shared and related changes to how locking happens

Fixes everywhere.

7 files changed, 112 insertions, 9 deletions

diff --git a/obnamlib/plugins/backup_plugin.py b/obnamlib/plugins/backup_plugin.py
index 50e279c1..d297404f 100644
--- a/obnamlib/plugins/backup_plugin.py
+++ b/obnamlib/plugins/backup_plugin.py
@@ -175,6 +175,7 @@ class BackupPlugin(obnamlib.ObnamPlugin):
             self.repo = self.app.open_repository(create=True)
             self.add_client(client_name)
             self.repo.lock_client(client_name)
+            self.repo.lock_shared()
 
         self.started = time.time()
         self.errors = False
@@ -188,6 +189,7 @@ class BackupPlugin(obnamlib.ObnamPlugin):
             self.app.ts['what'] = 'committing changes;'
             if not self.pretend:
                 self.repo.commit_client()
+                self.repo.commit_shared()
             self.repo.fs.close()
             self.app.ts.clear()
             self.report_stats()
@@ -198,6 +200,7 @@ class BackupPlugin(obnamlib.ObnamPlugin):
             if self.repo.got_client_lock:
                 logging.info('Unlocking client because of error')
                 self.repo.unlock_client()
+                self.repo.unlock_shared()
             raise
 
     def add_client(self, client_name):
@@ -289,7 +292,9 @@ class BackupPlugin(obnamlib.ObnamPlugin):
             self.checkpoints.append(self.repo.new_generation)
             self.backup_parents('.')
             self.repo.commit_client(checkpoint=True)
+            self.repo.commit_shared()
             self.repo.lock_client(self.app.settings['client-name'])
+            self.repo.lock_shared()
             self.repo.start_generation()
             self.last_checkpoint = self.repo.fs.bytes_written
             self.app.dump_memory_profile('at end of checkpoint')
diff --git a/obnamlib/plugins/forget_plugin.py b/obnamlib/plugins/forget_plugin.py
index a7d7ed85..8102635e 100644
--- a/obnamlib/plugins/forget_plugin.py
+++ b/obnamlib/plugins/forget_plugin.py
@@ -41,6 +41,7 @@ class ForgetPlugin(obnamlib.ObnamPlugin):
 
         self.repo = self.app.open_repository()
         self.repo.lock_client(self.app.settings['client-name'])
+        self.repo.lock_shared()
 
         self.app.dump_memory_profile('at beginning')
         if args:
@@ -72,6 +73,7 @@ class ForgetPlugin(obnamlib.ObnamPlugin):
                 self.app.dump_memory_profile('after removing %s' % genid)
 
         self.repo.commit_client()
+        self.repo.commit_shared()
         self.app.dump_memory_profile('after committing')
         self.repo.fs.close()
         self.app.ts.finish()
diff --git a/obnamlib/repo.py b/obnamlib/repo.py
index fe9e345b..f32d76a6 100644
--- a/obnamlib/repo.py
+++ b/obnamlib/repo.py
@@ -235,6 +235,21 @@ class Repository(object):
         if self.new_generation is None:
             raise obnamlib.Error('new generation has not started')
 
+    def require_no_root_lock(self):
+        '''Ensure we haven't locked root yet.'''
+        if self.got_root_lock:
+            raise obnamlib.Error('We have already locked root, oops')
+
+    def require_no_shared_lock(self):
+        '''Ensure we haven't locked shared B-trees yet.'''
+        if self.got_shared_lock:
+            raise obnamlib.Error('We have already locked shared B-trees, oops')
+
+    def require_no_client_lock(self):
+        '''Ensure we haven't locked the per-client B-tree yet.'''
+        if self.got_client_lock:
+            raise obnamlib.Error('We have already locked the client, oops')
+
     def lock_root(self):
         '''Lock root node.
         
@@ -243,7 +258,11 @@ class Repository(object):
         
         '''
 
-        tracing.trace('locking root')        
+        tracing.trace('locking root')
+        self.require_no_root_lock()
+        self.require_no_client_lock()
+        self.require_no_shared_lock()
+
         self.check_format_version()
         self.lockmgr.lock(['.'])
         self.got_root_lock = True
@@ -357,9 +376,22 @@ class Repository(object):
         '''
 
         tracing.trace('locking shared')
+        self.require_no_shared_lock()
         self.check_format_version()
         self.lockmgr.lock(self.shared_dirs)
         self.got_shared_lock = True
+        tracing.trace('starting changes in chunksums and chunklist')
+        self.chunksums.start_changes()
+        self.chunklist.start_changes()
+
+    def commit_shared(self):
+        '''Commit changes to shared B-trees.'''
+        
+        tracing.trace('committing shared')
+        self.require_shared_lock()
+        self.chunklist.commit()
+        self.chunksums.commit()
+        self.unlock_shared()
 
     def unlock_shared(self):
         '''Unlock currently locked shared B-trees.'''
@@ -377,6 +409,9 @@ class Repository(object):
         '''
 
         tracing.trace('client_name=%s', client_name)
+        self.require_no_client_lock()
+        self.require_no_shared_lock()
+        
         self.check_format_version()
         client_id = self.clientlist.get_client_id(client_name)
         if client_id is None:
@@ -418,14 +453,13 @@ class Repository(object):
         '''Commit changes to and unlock currently locked client.'''
         tracing.trace('committing client (checkpoint=%s)', checkpoint)
         self.require_client_lock()
+        self.require_shared_lock()
         if self.new_generation:
             self.client.set_current_generation_is_checkpoint(checkpoint)
         self.added_generations = []
         for genid in self.removed_generations:
             self._really_remove_generation(genid)
         self.client.commit()
-        self.chunklist.commit()
-        self.chunksums.commit()
         self.unlock_client()
         
     def open_client(self, client_name):
@@ -500,6 +534,7 @@ class Repository(object):
                     self.remove_chunk(chunk_id)
 
         self.require_client_lock()
+        self.require_shared_lock()
         logging.debug('_really_remove_generation: %d' % gen_id)
         chunk_ids = set(self.client.list_chunks_in_generation(gen_id))
         chunk_ids = filter_away_chunks_used_by_other_gens(chunk_ids, gen_id)
@@ -575,6 +610,8 @@ class Repository(object):
         
         tracing.trace('putting chunk (checksum=%s)', repr(checksum))
         self.require_started_generation()
+        self.require_shared_lock()
+
         if self.prev_chunkid is None:
             self.prev_chunkid = random_chunkid()
         while True:
@@ -641,6 +678,7 @@ class Repository(object):
 
         tracing.trace('chunk_id=%s', chunk_id)
         self.require_open_client()
+        self.require_shared_lock()
         self.chunklist.remove(chunk_id)
         filename = self._chunk_filename(chunk_id)
         try:
diff --git a/obnamlib/repo_tests.py b/obnamlib/repo_tests.py
index 1da63fd0..06213867 100644
--- a/obnamlib/repo_tests.py
+++ b/obnamlib/repo_tests.py
@@ -85,7 +85,7 @@ class RepositoryRootNodeTests(unittest.TestCase):
         
     def test_locking_root_node_twice_fails(self):
         self.repo.lock_root()
-        self.assertRaises(obnamlib.LockFail, self.repo.lock_root)
+        self.assertRaises(obnamlib.Error, self.repo.lock_root)
         
     def test_commit_releases_lock(self):
         self.repo.lock_root()
@@ -130,6 +130,10 @@ class RepositoryRootNodeTests(unittest.TestCase):
     def test_locks_shared(self):
         self.repo.lock_shared()
         self.assertTrue(self.repo.got_shared_lock)
+        
+    def test_locking_shared_twice_fails(self):
+        self.repo.lock_shared()
+        self.assertRaises(obnamlib.Error, self.repo.lock_shared)
 
     def test_unlocks_shared(self):
         self.repo.lock_shared()
@@ -222,13 +226,18 @@ class RepositoryRootNodeTests(unittest.TestCase):
         self.repo.lock_root()
         self.repo.add_client('foo')
         self.repo.commit_root()
+
         self.repo.lock_client('foo')
+        self.repo.lock_shared()
         self.repo.start_generation()
         self.repo.create('/', obnamlib.Metadata())
         self.repo.commit_client()
+        self.repo.commit_shared()
+
         self.repo.lock_root()
         self.repo.remove_client('foo')
         self.repo.commit_root()
+
         self.assertEqual(self.repo.list_clients(), [])
         self.assertFalse(self.fs.exists('foo'))
 
@@ -275,7 +284,7 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_locking_client_twice_fails(self):
         self.repo.lock_client('client_name')
-        self.assertRaises(obnamlib.LockFail, self.repo.lock_client, 
+        self.assertRaises(obnamlib.Error, self.repo.lock_client, 
                           'client_name')
 
     def test_locking_nonexistent_client_fails(self):
@@ -288,22 +297,29 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_commit_client_releases_lock(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.commit_client()
+        self.repo.commit_shared()
         self.assertFalse(self.repo.got_client_lock)
 
     def test_commit_does_not_mark_as_checkpoint_by_default(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.start_generation()
         genid = self.repo.new_generation
         self.repo.commit_client()
+        self.repo.commit_shared()
         self.repo.open_client('client_name')
         self.assertFalse(self.repo.get_is_checkpoint(genid))
 
     def test_commit_marks_as_checkpoint_when_requested(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.start_generation()
         genid = self.repo.new_generation
         self.repo.commit_client(checkpoint=True)
+        self.repo.commit_shared()
+
         self.repo.open_client('client_name')
         self.assert_(self.repo.get_is_checkpoint(genid))
 
@@ -360,8 +376,10 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_second_generation_has_different_id_from_first(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
         self.repo.lock_client('client_name')
         self.assertNotEqual(gen, self.repo.start_generation())
 
@@ -374,8 +392,11 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_commited_generation_has_start_and_end_times(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
+
         self.repo.open_client('client_name')
         start, end = self.repo.get_generation_times(gen)
         self.assertNotEqual(start, None)
@@ -384,43 +405,55 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_adding_generation_without_committing_does_not_add_it(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.start_generation()
         self.repo.unlock_client()
+        self.repo.unlock_shared()
         self.repo.open_client('client_name')
         self.assertEqual(self.repo.list_generations(), [])
 
     def test_removing_generation_works(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
 
         self.repo.open_client('client_name')
         self.assertEqual(len(self.repo.list_generations()), 1)
 
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.remove_generation(gen)
         self.repo.commit_client()
+        self.repo.commit_shared()
 
         self.repo.open_client('client_name')
         self.assertEqual(self.repo.list_generations(), [])
 
     def test_removing_only_second_generation_works(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen1 = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
 
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen2 = self.repo.start_generation()
         chunk_id = self.repo.put_chunk('data', self.repo.checksum('data'))
         self.repo.set_file_chunks('/foo', [chunk_id])
         self.repo.commit_client()
+        self.repo.commit_shared()
 
         self.repo.open_client('client_name')
         self.assertEqual(len(self.repo.list_generations()), 2)
 
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.remove_generation(gen2)
         self.repo.commit_client()
+        self.repo.commit_shared()
 
         self.repo.open_client('client_name')
         self.assertEqual(self.repo.list_generations(), [gen1])
@@ -434,11 +467,17 @@ class RepositoryClientTests(unittest.TestCase):
 
     def test_removing_without_committing_does_not_remove(self):
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         gen = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
+
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.repo.remove_generation(gen)
         self.repo.unlock_client()
+        self.repo.unlock_shared()
+
         self.repo.open_client('client_name')
         self.assertEqual(self.repo.list_generations(), [gen])
 
@@ -551,9 +590,11 @@ class RepositoryChunkTests(unittest.TestCase):
         self.assertNotEqual(self.repo.checksum('data'), None)
 
     def test_put_chunk_returns_id(self):
+        self.repo.lock_shared()
         self.assertNotEqual(self.repo.put_chunk('data', 'checksum'), None)
         
     def test_get_chunk_retrieves_what_put_chunk_puts(self):
+        self.repo.lock_shared()
         chunkid = self.repo.put_chunk('data', 'checksum')
         self.assertEqual(self.repo.get_chunk(chunkid), 'data')
         
@@ -561,18 +602,22 @@ class RepositoryChunkTests(unittest.TestCase):
         self.assertFalse(self.repo.chunk_exists(1234))
         
     def test_chunk_exists_after_it_is_put(self):
+        self.repo.lock_shared()
         chunkid = self.repo.put_chunk('chunk', 'checksum')
         self.assert_(self.repo.chunk_exists(chunkid))
 
     def test_removes_chunk(self):
+        self.repo.lock_shared()
         chunkid = self.repo.put_chunk('chunk', 'checksum')
         self.repo.remove_chunk(chunkid)
         self.assertFalse(self.repo.chunk_exists(chunkid))
 
     def test_silently_ignores_failure_when_removing_nonexistent_chunk(self):
+        self.repo.lock_shared()
         self.assertEqual(self.repo.remove_chunk(0), None)
         
     def test_find_chunks_finds_what_put_chunk_puts(self):
+        self.repo.lock_shared()
         checksum = self.repo.checksum('data')
         chunkid = self.repo.put_chunk('data', checksum)
         self.assertEqual(self.repo.find_chunks(checksum), [chunkid])
@@ -581,6 +626,7 @@ class RepositoryChunkTests(unittest.TestCase):
         self.assertEqual(self.repo.find_chunks('checksum'), [])
         
     def test_handles_checksum_collision(self):
+        self.repo.lock_shared()
         checksum = self.repo.checksum('data')
         chunkid1 = self.repo.put_chunk('data', checksum)
         chunkid2 = self.repo.put_chunk('data', checksum)
@@ -591,6 +637,7 @@ class RepositoryChunkTests(unittest.TestCase):
         self.assertEqual(self.repo.list_chunks(), [])
         
     def test_returns_chunks_after_they_exist(self):
+        self.repo.lock_shared()
         checksum = self.repo.checksum('data')
         chunkids = []
         for i in range(2):
@@ -660,6 +707,7 @@ class RepositoryGenspecTests(unittest.TestCase):
         self.repo.add_client('client_name')
         self.repo.commit_root()
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
@@ -667,7 +715,9 @@ class RepositoryGenspecTests(unittest.TestCase):
     def backup(self):
         gen = self.repo.start_generation()
         self.repo.commit_client()
+        self.repo.commit_shared()
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         return gen
 
     def test_latest_raises_error_if_there_are_no_generations(self):
@@ -720,6 +770,7 @@ class RepositoryWalkTests(unittest.TestCase):
         self.file_meta.st_mode = stat.S_IFREG | 0644
         
         self.repo.lock_client('client_name')
+        self.repo.lock_shared()
         self.gen = self.repo.start_generation()
         
         self.repo.create('/', self.dir_meta)
diff --git a/obnamlib/repo_tree.py b/obnamlib/repo_tree.py
index ad7bb23d..87b4cc81 100644
--- a/obnamlib/repo_tree.py
+++ b/obnamlib/repo_tree.py
@@ -65,9 +65,13 @@ class RepositoryTree(object):
 
     def start_changes(self):
         tracing.trace('start changes for %s', self.dirname)
-        if not self.fs.exists(self.dirname):
-            tracing.trace('create %s', self.dirname)
-            self.fs.mkdir(self.dirname)
+        need_init = (not self.fs.exists(self.dirname) or
+                     self.fs.listdir(self.dirname) == [] or
+                     self.fs.listdir(self.dirname) == ['lock'])
+        if need_init:
+            if not self.fs.exists(self.dirname):
+                tracing.trace('create %s', self.dirname)
+                self.fs.mkdir(self.dirname)
             self.repo.hooks.call('repository-toplevel-init', self.repo, 
                                  self.dirname)
         self.init_forest()
diff --git a/obnamlib/vfs_local.py b/obnamlib/vfs_local.py
index 04323b94..ea608e8a 100644
--- a/obnamlib/vfs_local.py
+++ b/obnamlib/vfs_local.py
@@ -192,6 +192,7 @@ class LocalFS(obnamlib.VirtualFileSystem):
         tracing.trace('pathname=%s', pathname)
         tracing.trace('mode=%s', mode)
         f = LocalFSFile(self.join(pathname), mode)
+        tracing.trace('opened %s', pathname)
         try:
             flags = fcntl.fcntl(f.fileno(), fcntl.F_GETFL)
             flags |= os.O_NOATIME
@@ -199,6 +200,7 @@ class LocalFS(obnamlib.VirtualFileSystem):
         except IOError, e: # pragma: no cover
             tracing.trace('fcntl F_SETFL failed: %s', repr(e))
             return f # ignore any problems setting flags
+        tracing.trace('returning ok')
         return f
 
     def exists(self, pathname):
@@ -282,6 +284,7 @@ class LocalFS(obnamlib.VirtualFileSystem):
         path = self.join(pathname)
         dirname = os.path.dirname(path)
         if not os.path.exists(dirname):
+            tracing.trace('os.makedirs(%s)' % dirname)
             os.makedirs(dirname)
 
         fd, tempname = tempfile.mkstemp(dir=dirname)
diff --git a/tests/compression+encryption.script b/tests/compression+encryption.script
index cb387290..d61dc64d 100755
--- a/tests/compression+encryption.script
+++ b/tests/compression+encryption.script
@@ -17,7 +17,7 @@
 set -e
 
 gpgkey='3B1802F81B321347'
-$SRCDIR/tests/backup --encrypt-with="$gpgkey" --compress-with=gzip
+$SRCDIR/tests/backup --encrypt-with="$gpgkey" --compress-with=gzip --log=/tmp/foo.log
 $SRCDIR/tests/restore --encrypt-with="$gpgkey" --compress-with=gzip
 $SRCDIR/tests/verify