Use lockf() instead of flock() when locking repository/cache

This is a reworked version of Petros Moisiadis' original pull request
since some extra effort is needed to support access to read-only
repositories.

CHANGES

@@ -8,6 +8,7 @@ Version 0.9
 
 (feature release, released on X)
 
+- Use fcntl() instead of flock() when locking repository/cache. (#15)
 - Let ssh figure out port/user if not specified so we don't override .ssh/config (#9)
 
 Version 0.8.1

attic/cache.py

@@ -1,12 +1,11 @@
 from configparser import RawConfigParser
-import fcntl
 from itertools import zip_longest
 import msgpack
 import os
 from binascii import hexlify
 import shutil
 
-from .helpers import get_cache_dir, decode_dict, st_mtime_ns, unhexlify
+from .helpers import get_cache_dir, decode_dict, st_mtime_ns, unhexlify, UpgradableLock
 from .hashindex import ChunkIndex
 
 
@@ -58,8 +57,7 @@ class Cache(object):
     def open(self):
         if not os.path.isdir(self.path):
             raise Exception('%s Does not look like an Attic cache' % self.path)
-        self.lock_fd = open(os.path.join(self.path, 'README'), 'r+')
-        fcntl.flock(self.lock_fd, fcntl.LOCK_EX)
+        self.lock = UpgradableLock(os.path.join(self.path, 'config'), exclusive=True)
         self.rollback()
         self.config = RawConfigParser()
         self.config.read(os.path.join(self.path, 'config'))
@@ -72,7 +70,7 @@ class Cache(object):
         self.files = None
 
     def close(self):
-        self.lock_fd.close()
+        self.lock.release()
 
     def _read_files(self):
         self.files = {}

attic/helpers.py

@@ -11,6 +11,29 @@ import time
 from datetime import datetime, timedelta
 from fnmatch import fnmatchcase
 from operator import attrgetter
+import fcntl
+
+
+class UpgradableLock:
+
+    def __init__(self, path, exclusive=False):
+        try:
+            self.fd = open(path, 'r+')
+        except IOError:
+            self.fd = open(path, 'r')
+        if exclusive:
+            fcntl.lockf(self.fd, fcntl.LOCK_EX)
+        else:
+            fcntl.lockf(self.fd, fcntl.LOCK_SH)
+        self.is_exclusive = exclusive
+
+    def upgrade(self):
+        fcntl.lockf(self.fd, fcntl.LOCK_EX)
+        self.is_exclusive = True
+
+    def release(self):
+        fcntl.lockf(self.fd, fcntl.LOCK_UN)
+        self.fd.close()
 
 
 class Manifest:

attic/repository.py

@@ -1,6 +1,5 @@
 from configparser import RawConfigParser
 from binascii import hexlify
-import fcntl
 import os
 import re
 import shutil
@@ -8,7 +7,7 @@ import struct
 from zlib import crc32
 
 from .hashindex import NSIndex
-from .helpers import IntegrityError, read_msgpack, write_msgpack, unhexlify
+from .helpers import IntegrityError, read_msgpack, write_msgpack, unhexlify, UpgradableLock
 from .lrucache import LRUCache
 
 MAX_OBJECT_SIZE = 20 * 1024 * 1024
@@ -39,7 +38,7 @@ class Repository(object):
 
     def __init__(self, path, create=False):
         self.io = None
-        self.lock_fd = None
+        self.lock = None
         if create:
             self.create(path)
         self.open(path)
@@ -71,8 +70,7 @@ class Repository(object):
         self.path = path
         if not os.path.isdir(path):
             raise self.DoesNotExist(path)
-        self.lock_fd = open(os.path.join(path, 'config'), 'r')
-        fcntl.flock(self.lock_fd, fcntl.LOCK_EX)
+        self.lock = UpgradableLock(os.path.join(path, 'config'))
         self.config = RawConfigParser()
         self.config.read(os.path.join(self.path, 'config'))
         if self.config.getint('repository', 'version') != 1:
@@ -83,10 +81,10 @@ class Repository(object):
         self.rollback()
 
     def close(self):
-        if self.lock_fd:
+        if self.lock:
             self.rollback()
-            self.lock_fd.close()
-            self.lock_fd = None
+            self.lock.release()
+            self.lock = None
 
     def commit(self, rollback=True):
         """Commit transaction
@@ -202,6 +200,7 @@ class Repository(object):
             self.io.close()
         self.io = LoggedIO(self.path, self.max_segment_size, self.segments_per_dir)
         if self.io.head is not None and not os.path.exists(os.path.join(self.path, 'index.%d' % self.io.head)):
+            self.lock.upgrade()
             self.recover(self.path)
         self.open_index(self.io.head, read_only=True)
 
@@ -222,6 +221,7 @@ class Repository(object):
     def put(self, id, data, wait=True):
         if not self._active_txn:
             self._active_txn = True
+            self.lock.upgrade()
             self.open_index(self.io.head)
         try:
             segment, _ = self.index[id]
@@ -240,6 +240,7 @@ class Repository(object):
     def delete(self, id, wait=True):
         if not self._active_txn:
             self._active_txn = True
+            self.lock.upgrade()
             self.open_index(self.io.head)
         try:
             segment, offset = self.index.pop(id)

attic/testsuite/helpers.py

@@ -1,5 +1,8 @@
 from datetime import datetime
-from attic.helpers import Location, format_timedelta, IncludePattern, ExcludePattern, make_path_safe
+import os
+import tempfile
+import unittest
+from attic.helpers import Location, format_timedelta, IncludePattern, ExcludePattern, make_path_safe, UpgradableLock
 from attic.testsuite import AtticTestCase
 
 
@@ -66,3 +69,20 @@ class MakePathSafeTestCase(AtticTestCase):
         self.assert_equal(make_path_safe('/'), '.')
         self.assert_equal(make_path_safe('/'), '.')
 
+
+class UpgradableLockTestCase(AtticTestCase):
+
+    def test(self):
+        file = tempfile.NamedTemporaryFile()
+        lock = UpgradableLock(file.name)
+        lock.upgrade()
+        lock.upgrade()
+        lock.release()
+
+    @unittest.skipIf(os.getuid() == 0, 'Root can always open files for writing')
+    def test_read_only_lock_file(self):
+        file = tempfile.NamedTemporaryFile()
+        os.chmod(file.name, 0o444)
+        lock = UpgradableLock(file.name)
+        self.assert_raises(OSError, lock.upgrade)
+        lock.release()