Patchwork [21,of,21,V2] speedy: detect store corruption

login
register
mail settings
Submitter Tomasz Kleczek
Date Dec. 14, 2012, 2:52 a.m.
Message ID <f4f4e643e507047f03f5.1355453553@dev408.prn1.facebook.com>
Download mbox | patch
Permalink /patch/103/
State Deferred, archived
Headers show

Comments

Tomasz Kleczek - Dec. 14, 2012, 2:52 a.m.
# HG changeset patch
# User Tomasz Kleczek <tkleczek at fb.com>
# Date 1355361137 28800
# Node ID f4f4e643e507047f03f5538b8e7f528289f22465
# Parent  91413197d908e7db75c89f1dd23f41d37a0d02eb
speedy: detect store corruption

Index database might get into inconsistent state if the update operation is
interrupted. To detect such situations we use a simple file-based lock.
The lock is acquired before indice initialization and released once the
update operation completes successfully.
If the operation is interrupted the lock is not released. This will be detect
on the next indice initialization and all the indices will be rebuilt.

A simple custom lock implementation is added. The mercurial.lock could
not be reused as it has different semantics:
  * It would allow to acquire an already-acquired lock if it detected
    that the process who owns the lock is dead (this is the exact case
    in which we would rather like it to fail.
  * The release() on an unacquired lock does not remove the LOCK file,
    even if it is present.

Patch

diff --git a/hgext/speedy/server.py b/hgext/speedy/server.py
--- a/hgext/speedy/server.py
+++ b/hgext/speedy/server.py
@@ -147,19 +147,45 @@ 
         'deerialize': serialize.identity,
         },
 }
-def makeserver(repo):
-    """Return an initialized metaserver instance.
 
-    Updates all indices to the last revision along the way.
+def makeindexstores(repo):
+    """Return a list with the initialized indice stores.
+
+    Updates all indices to the tip of the serverrepo.
+
+    If the store appears to be in an inconsistent state, rebuilds it.
+
+    A simple locking mechanism is used to ensure the consistency of the store.
+    Before modifying the store a lock is acquired (the LOCK file is created
+    on disk). This lock is later released (the file removed) only if
+    the transaction completes successfully.
+    If the lock could not be acquired (it was already taken by someone else) it
+    is very likely that previous transction was aborted and the store is not
+    in the consistent state. In this case all indices are erased and recreated.
     """
     opener = scmutil.opener(os.path.join(repo.path, 'hgext/speedyserver'))
     opener.makedirs()
-    lastrev = len(repo) - 1
-    indices = {}
+    lock = store.lock(os.path.join(repo.path, 'hgext/speedyserver/LOCK'))
+    recreate = False
+    try:
+        lock.acquire()
+    except store.LockHeld:
+        recreate = True
+    istores = []
+    updaterev = len(repo) - 1
     for name, cfg in indicecfg.iteritems():
         istore = store.indexstore(name, opener, **cfg)
-        istore.update(repo, lastrev)
-        indices[name] = istore.view()
+        if recreate:
+            istore.clear()
+        istore.update(repo, updaterev)
+        istores.append(istore)
+    lock.release()
+    return istores
+
+def makeserver(repo):
+    """Return an initialized metaserver instance."""
+    istores = makeindexstores(repo)
+    indices = dict([(istore.name, istore.view()) for istore in istores])
     return metaserver(repo, indices)
 
 @command('metaserve', [], _(''))
@@ -174,4 +200,3 @@ 
     serviceopts = collections.defaultdict(lambda: '')
     ui.status(_('listening on port %d\n' % port))
     cmdutil.service(serviceopts, runfn=server.serve_forever)
-
diff --git a/hgext/speedy/store.py b/hgext/speedy/store.py
--- a/hgext/speedy/store.py
+++ b/hgext/speedy/store.py
@@ -57,3 +57,25 @@ 
     def view(self):
         """Return a dict-like object for read access to the index elements."""
         return self._dict
+
+class LockHeld(Exception):
+    """Raised by `lock.acquire` if the lock is already taken."""
+    pass
+
+class lock(object):
+    """Simple file-based lock used to ensure store consistency."""
+
+    def __init__(self, path):
+        self.path = path
+
+    def acquire(self):
+        try:
+            ld = os.open(self.path, os.O_CREAT | os.O_WRONLY | os.O_EXCL)
+            os.close(ld)
+        except (OSError, IOError), why:
+            if why.errno == errno.EEXIST:
+                raise LockHeld
+            raise
+
+    def release(self):
+        os.remove(self.path)
diff --git a/tests/test-speedy.t b/tests/test-speedy.t
--- a/tests/test-speedy.t
+++ b/tests/test-speedy.t
@@ -235,6 +235,20 @@ 
 
   $ cd ..
 
+Testing store corruption detection
+
+  $ cd $TESTTMP/serverrepo
+
+  $ touch .hg/hgext/speedyserver/LOCK
+  $ rm -rf .hg/hgext/speedyserver/files .hg/hgext/speedyserver/filechgs
+
+  $ cd $TESTTMP/localrepo
+
+  $ hg log "glob:d2/*" --exclude "**.py"
+  chgx
+  chgpushed
+  chg4
+
 Testing socket server
 
 Writing server config file
@@ -364,3 +378,4 @@ 
   $ kill `cat pidfile` 2> /dev/null
   $ cat err
   handling request
+