Re: Concurrent pushes updating the same ref

[Jeff King <peff@peff.net>]

On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:

> fatal: Unable to create
> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.
> If no other git process is currently running, this probably means a
> git process crashed in this repository earlier. Make sure no other git
> process is running and remove the file manually to continue.
> fatal: The remote end hung up unexpectedly
> 
> I think the cause is pretty obvious, and in a normal interactive situation
> the solution would be to simply try again.  But in a script trying again
> isn't so straightforward.
> 
> So I'm wondering if there's any sense or desire to make git a little more
> flexible here.  Maybe teach it to wait and try again once or twice when it
> sees a lock file.  I presume that normally a ref lock file should disappear
> pretty quickly, so there shouldn't be a need to wait very long.

Yeah, we probably should try again. The simplest possible (and untested)
patch is below. However, a few caveats:

  1. This patch unconditionally retries for all lock files. Do all
     callers want that? I wonder if there are any exploratory lock
     acquisitions that would rather return immediately than have some
     delay.

  2. The number of tries and sleep time are pulled out of a hat.

  3. Even with retries, I don't know if you will get the behavior you
     want. The lock procedure for refs is:

        1. get the lock
        2. check and remember the sha1
        3. release the lock
        4. do some long-running work (like the actual push)
        5. get the lock
        6. check that the sha1 is the same as the remembered one
        7. update the sha1
        8. release the lock

     Right now you are getting contention on the lock itself. But may
     you not also run afoul of step (6) above? That is, one push updates
     the ref from A to B, then the other one in attempting to go from A
     to B sees that it has already changed to B under our feet and
     complains?

     I can certainly think of a rule around that special case (if we are
     going to B, and it already changed to B, silently leave it alone
     and pretend we wrote it), but I don't know how often that would be
     useful in the real world.

Anyway, patch (for discussion, not inclusion) is below.

diff --git a/lockfile.c b/lockfile.c
index b0d74cd..3329719 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -122,7 +122,7 @@ static char *resolve_symlink(char *p, size_t s)
 }
 
 
-static int lock_file(struct lock_file *lk, const char *path, int flags)
+static int lock_file_single(struct lock_file *lk, const char *path, int flags)
 {
 	if (strlen(path) >= sizeof(lk->filename))
 		return -1;
@@ -155,6 +155,21 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	return lk->fd;
 }
 
+static int lock_file(struct lock_file *lk, const char *path, int flags)
+{
+	int tries;
+	int fd;
+	for (tries = 0; tries < 3; tries++) {
+		fd = lock_file_single(lk, path, flags);
+		if (fd >= 0)
+			return fd;
+		if (errno != EEXIST)
+			return fd;
+		sleep(1);
+	}
+	return fd;
+}
+
 static char *unable_to_lock_message(const char *path, int err)
 {
 	struct strbuf buf = STRBUF_INIT;