{"thread":{"id":"26214","subject":"Concurrent pushes updating the same ref","startedAt":"2011-01-06T15:46:38Z","lastAt":"2011-01-10T22:14:01Z","messageCount":8,"participants":["Marc Branchaud","Jeff King","Shawn Pearce","Ilari Liusvaara","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159032","messageId":"4D25E3DE.7050801@xiplink.com","threadId":"26214","inReplyTo":null,"subject":"Concurrent pushes updating the same ref","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-01-06T15:46:38Z","receivedAt":"2011-01-06T15:46:38Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"Hi all,\n\n[ BACKGROUND: I've modified our build system to push a custom ref at the\nstart of each build.  The aim is to identify in the repo which revision got\nbuilt.  For us, an overall \"build\" consists of creating about a dozen\nproducts, all from the same source tree.  The build system (Hudson) launches\neach product's build concurrently on one or more build slaves.  Each of those\nindividual product builds clones the repo, checks out the appropriate\nrevision, and pushes up the custom ref.  (I would have liked to make the\nHudson master job push up the ref, instead of all the slave jobs, but I\ncouldn't find a way to do that.) ]\n\nUsually this works:  Each slave is setting the ref to the same value, so the\norder of the updates doesn't matter.  But every once in a while, the push\nfails with:\n\nfatal: Unable to create\n'/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\nIf no other git process is currently running, this probably means a\ngit process crashed in this repository earlier. Make sure no other git\nprocess is running and remove the file manually to continue.\nfatal: The remote end hung up unexpectedly\n\nI think the cause is pretty obvious, and in a normal interactive situation\nthe solution would be to simply try again.  But in a script trying again\nisn't so straightforward.\n\nSo I'm wondering if there's any sense or desire to make git a little more\nflexible here.  Maybe teach it to wait and try again once or twice when it\nsees a lock file.  I presume that normally a ref lock file should disappear\npretty quickly, so there shouldn't be a need to wait very long.\n\nThoughts?\n\n\t\tM.\n"},{"id":"159034","messageId":"20110106163035.GA7812@sigill.intra.peff.net","threadId":"26214","inReplyTo":"4D25E3DE.7050801@xiplink.com","subject":"Re: Concurrent pushes updating the same ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-06T16:30:35Z","receivedAt":"2011-01-06T16:30:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:\n\n> fatal: Unable to create\n> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\n> If no other git process is currently running, this probably means a\n> git process crashed in this repository earlier. Make sure no other git\n> process is running and remove the file manually to continue.\n> fatal: The remote end hung up unexpectedly\n> \n> I think the cause is pretty obvious, and in a normal interactive situation\n> the solution would be to simply try again.  But in a script trying again\n> isn't so straightforward.\n> \n> So I'm wondering if there's any sense or desire to make git a little more\n> flexible here.  Maybe teach it to wait and try again once or twice when it\n> sees a lock file.  I presume that normally a ref lock file should disappear\n> pretty quickly, so there shouldn't be a need to wait very long.\n\nYeah, we probably should try again. The simplest possible (and untested)\npatch is below. However, a few caveats:\n\n  1. This patch unconditionally retries for all lock files. Do all\n     callers want that? I wonder if there are any exploratory lock\n     acquisitions that would rather return immediately than have some\n     delay.\n\n  2. The number of tries and sleep time are pulled out of a hat.\n\n  3. Even with retries, I don't know if you will get the behavior you\n     want. The lock procedure for refs is:\n\n        1. get the lock\n        2. check and remember the sha1\n        3. release the lock\n        4. do some long-running work (like the actual push)\n        5. get the lock\n        6. check that the sha1 is the same as the remembered one\n        7. update the sha1\n        8. release the lock\n\n     Right now you are getting contention on the lock itself. But may\n     you not also run afoul of step (6) above? That is, one push updates\n     the ref from A to B, then the other one in attempting to go from A\n     to B sees that it has already changed to B under our feet and\n     complains?\n\n     I can certainly think of a rule around that special case (if we are\n     going to B, and it already changed to B, silently leave it alone\n     and pretend we wrote it), but I don't know how often that would be\n     useful in the real world.\n\nAnyway, patch (for discussion, not inclusion) is below.\n\ndiff --git a/lockfile.c b/lockfile.c\nindex b0d74cd..3329719 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -122,7 +122,7 @@ static char *resolve_symlink(char *p, size_t s)\n }\n \n \n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file_single(struct lock_file *lk, const char *path, int flags)\n {\n \tif (strlen(path) >= sizeof(lk->filename))\n \t\treturn -1;\n@@ -155,6 +155,21 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \treturn lk->fd;\n }\n \n+static int lock_file(struct lock_file *lk, const char *path, int flags)\n+{\n+\tint tries;\n+\tint fd;\n+\tfor (tries = 0; tries < 3; tries++) {\n+\t\tfd = lock_file_single(lk, path, flags);\n+\t\tif (fd >= 0)\n+\t\t\treturn fd;\n+\t\tif (errno != EEXIST)\n+\t\t\treturn fd;\n+\t\tsleep(1);\n+\t}\n+\treturn fd;\n+}\n+\n static char *unable_to_lock_message(const char *path, int err)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n"},{"id":"159035","messageId":"AANLkTinGYjExrnHCqcTPu-APzk9WynxwWoKVOLaSkZ2y@mail.gmail.com","threadId":"26214","inReplyTo":"20110106163035.GA7812@sigill.intra.peff.net","subject":"Re: Concurrent pushes updating the same ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-01-06T16:48:11Z","receivedAt":"2011-01-06T16:48:11Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Jan 6, 2011 at 08:30, Jeff King <peff@peff.net> wrote:\n>\n> Yeah, we probably should try again. The simplest possible (and untested)\n> patch is below. However, a few caveats:\n>\n>  1. This patch unconditionally retries for all lock files. Do all\n>     callers want that? I wonder if there are any exploratory lock\n>     acquisitions that would rather return immediately than have some\n>     delay.\n\nI don't see why not.  We shouldn't be exploring to see if a lock is\npossible anywhere.\n\n>  2. The number of tries and sleep time are pulled out of a hat.\n\nFWIW, JGit has started to do some of this stuff for Windows.  We're\nusing 10 retries, with a delay of 100 milliseconds between each.  This\nwas also pulled out of a hat, but it seems to have resolved the bug\nreports that came in on Windows.  We unfortunately have to do retries\non directory and file deletion.\n\n>  3. Even with retries, I don't know if you will get the behavior you\n>     want. The lock procedure for refs is:\n>\n>        1. get the lock\n>        2. check and remember the sha1\n>        3. release the lock\n\nWhy are we locking the ref to read it?  You can read a ref atomically\nwithout locking.\n\n>        4. do some long-running work (like the actual push)\n>        5. get the lock\n>        6. check that the sha1 is the same as the remembered one\n>        7. update the sha1\n>        8. release the lock\n>\n>     Right now you are getting contention on the lock itself. But may\n>     you not also run afoul of step (6) above? That is, one push updates\n>     the ref from A to B, then the other one in attempting to go from A\n>     to B sees that it has already changed to B under our feet and\n>     complains?\n\nNot if its a force push.  :-)\n\n-- \nShawn.\n"},{"id":"159040","messageId":"4D25F80F.3050604@xiplink.com","threadId":"26214","inReplyTo":"20110106163035.GA7812@sigill.intra.peff.net","subject":"Re: Concurrent pushes updating the same ref","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-01-06T17:12:47Z","receivedAt":"2011-01-06T17:12:47Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-01-06 11:30 AM, Jeff King wrote:\n> On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:\n> \n>> fatal: Unable to create\n>> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\n>> If no other git process is currently running, this probably means a\n>> git process crashed in this repository earlier. Make sure no other git\n>> process is running and remove the file manually to continue.\n>> fatal: The remote end hung up unexpectedly\n>>\n>> I think the cause is pretty obvious, and in a normal interactive situation\n>> the solution would be to simply try again.  But in a script trying again\n>> isn't so straightforward.\n>>\n>> So I'm wondering if there's any sense or desire to make git a little more\n>> flexible here.  Maybe teach it to wait and try again once or twice when it\n>> sees a lock file.  I presume that normally a ref lock file should disappear\n>> pretty quickly, so there shouldn't be a need to wait very long.\n> \n> Yeah, we probably should try again. The simplest possible (and untested)\n> patch is below. However, a few caveats:\n> \n>   1. This patch unconditionally retries for all lock files. Do all\n>      callers want that? I wonder if there are any exploratory lock\n>      acquisitions that would rather return immediately than have some\n>      delay.\n> \n>   2. The number of tries and sleep time are pulled out of a hat.\n> \n>   3. Even with retries, I don't know if you will get the behavior you\n>      want. The lock procedure for refs is:\n> \n>         1. get the lock\n>         2. check and remember the sha1\n>         3. release the lock\n>         4. do some long-running work (like the actual push)\n>         5. get the lock\n>         6. check that the sha1 is the same as the remembered one\n>         7. update the sha1\n>         8. release the lock\n> \n>      Right now you are getting contention on the lock itself. But may\n>      you not also run afoul of step (6) above? That is, one push updates\n>      the ref from A to B, then the other one in attempting to go from A\n>      to B sees that it has already changed to B under our feet and\n>      complains?\n\nCould not anything run afoul of step (6)?  Who knows what might happen in\nstep (4)...\n\nHowever, in my particular case I'm using a \"force\" refspec:\n\n\tgit push origin +HEAD:refs/builds/${TAG}\n\nso (as Shawn says) step (6) shouldn't matter, right?  Plus, all the\nconcurrent pushes are setting the ref to the same value anyway.\n\nThis is fairly degenerate behaviour though.\n\n>      I can certainly think of a rule around that special case (if we are\n>      going to B, and it already changed to B, silently leave it alone\n>      and pretend we wrote it), but I don't know how often that would be\n>      useful in the real world.\n\nYes -- useful in my case, but otherwise...  Still, I think it would be\nmore-correct to do that.\n\n\t\tM.\n"},{"id":"159042","messageId":"20110106172848.GA15729@LK-Perkele-VI.localdomain","threadId":"26214","inReplyTo":"AANLkTinGYjExrnHCqcTPu-APzk9WynxwWoKVOLaSkZ2y@mail.gmail.com","subject":"Re: Concurrent pushes updating the same ref","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2011-01-06T17:28:48Z","receivedAt":"2011-01-06T17:28:48Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Jan 06, 2011 at 08:48:11AM -0800, Shawn Pearce wrote:\n> On Thu, Jan 6, 2011 at 08:30, Jeff King <peff@peff.net> wrote:\n> >\n> >     Right now you are getting contention on the lock itself. But may\n> >     you not also run afoul of step (6) above? That is, one push updates\n> >     the ref from A to B, then the other one in attempting to go from A\n> >     to B sees that it has already changed to B under our feet and\n> >     complains?\n> \n> Not if its a force push.  :-)\n\nIIRC, there are no wire protocol bits to denote a forced push, the\nforce option only overrides client-side checks. Thus, even forced pushes\ncan fail due to race conditions...\n\n-Ilari\n"},{"id":"159054","messageId":"7v1v4pbz6y.fsf@alter.siamese.dyndns.org","threadId":"26214","inReplyTo":"20110106163035.GA7812@sigill.intra.peff.net","subject":"Re: Concurrent pushes updating the same ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-06T19:37:57Z","receivedAt":"2011-01-06T19:37:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:\n>\n>> fatal: Unable to create\n>> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\n>> If no other git process is currently running, this probably means a\n>> git process crashed in this repository earlier. Make sure no other git\n>> process is running and remove the file manually to continue.\n>> fatal: The remote end hung up unexpectedly\n>> \n>> I think the cause is pretty obvious, and in a normal interactive situation\n>> the solution would be to simply try again.  But in a script trying again\n>> isn't so straightforward.\n>> \n>> So I'm wondering if there's any sense or desire to make git a little more\n>> flexible here.  Maybe teach it to wait and try again once or twice when it\n>> sees a lock file.  I presume that normally a ref lock file should disappear\n>> pretty quickly, so there shouldn't be a need to wait very long.\n>\n> Yeah, we probably should try again. The simplest possible (and untested)\n> patch is below. However, a few caveats:\n>\n>   1. This patch unconditionally retries for all lock files. Do all\n>      callers want that?\n\nI actually have to say that _no_ caller should want this.  If somebody\nearlier crashed, we would want to know about it (and how).  If somebody\nelse alive is actively holding a lock, why not make it the responsibility\nof a calling script to decide if it wants to retry itself or perhaps\ndecide to do something else?\n"},{"id":"159077","messageId":"4D263979.1080403@xiplink.com","threadId":"26214","inReplyTo":"7v1v4pbz6y.fsf@alter.siamese.dyndns.org","subject":"Re: Concurrent pushes updating the same ref","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-01-06T21:51:53Z","receivedAt":"2011-01-06T21:51:53Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-01-06 02:37 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:\n>>\n>>> fatal: Unable to create\n>>> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\n>>> If no other git process is currently running, this probably means a\n>>> git process crashed in this repository earlier. Make sure no other git\n>>> process is running and remove the file manually to continue.\n>>> fatal: The remote end hung up unexpectedly\n>>>\n>>> I think the cause is pretty obvious, and in a normal interactive situation\n>>> the solution would be to simply try again.  But in a script trying again\n>>> isn't so straightforward.\n>>>\n>>> So I'm wondering if there's any sense or desire to make git a little more\n>>> flexible here.  Maybe teach it to wait and try again once or twice when it\n>>> sees a lock file.  I presume that normally a ref lock file should disappear\n>>> pretty quickly, so there shouldn't be a need to wait very long.\n>>\n>> Yeah, we probably should try again. The simplest possible (and untested)\n>> patch is below. However, a few caveats:\n>>\n>>   1. This patch unconditionally retries for all lock files. Do all\n>>      callers want that?\n> \n> I actually have to say that _no_ caller should want this.  If somebody\n> earlier crashed, we would want to know about it (and how).  If somebody\n> else alive is actively holding a lock, why not make it the responsibility\n> of a calling script to decide if it wants to retry itself or perhaps\n> decide to do something else?\n\nI'm not sure I follow this.\n\nHow would retrying a few times prevent us from finding out about an earlier\ncrash?  It's not like we're overriding the lock by retrying.  Nobody's going\nto be able to remove a lock created by a crashed process, right?\n\nAnd if someone active doesn't release the lock and the low-level code retried\na few times, the caller can still decide what to do.  I don't see how it\nwould even impact that decision -- if the caller wants to try again, the\nsystem can still retry a few times underneath the caller's one retry.  It\nseems fine to me.\n\n\t\tM.\n"},{"id":"159294","messageId":"4D2B84A9.4040603@xiplink.com","threadId":"26214","inReplyTo":"4D25F80F.3050604@xiplink.com","subject":"Re: Concurrent pushes updating the same ref","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-01-10T22:14:01Z","receivedAt":"2011-01-10T22:14:01Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-01-06 12:12 PM, Marc Branchaud wrote:\n> On 11-01-06 11:30 AM, Jeff King wrote:\n>> On Thu, Jan 06, 2011 at 10:46:38AM -0500, Marc Branchaud wrote:\n>>\n>>> fatal: Unable to create\n>>> '/usr/xiplink/git/public/Main.git/refs/builds/3.3.0-3.lock': File exists.\n>>> If no other git process is currently running, this probably means a\n>>> git process crashed in this repository earlier. Make sure no other git\n>>> process is running and remove the file manually to continue.\n>>> fatal: The remote end hung up unexpectedly\n>>>\n>>> I think the cause is pretty obvious, and in a normal interactive situation\n>>> the solution would be to simply try again.  But in a script trying again\n>>> isn't so straightforward.\n>>>\n>>> So I'm wondering if there's any sense or desire to make git a little more\n>>> flexible here.  Maybe teach it to wait and try again once or twice when it\n>>> sees a lock file.  I presume that normally a ref lock file should disappear\n>>> pretty quickly, so there shouldn't be a need to wait very long.\n>>\n>> Yeah, we probably should try again. The simplest possible (and untested)\n>> patch is below. However, a few caveats:\n>>\n>>   1. This patch unconditionally retries for all lock files. Do all\n>>      callers want that? I wonder if there are any exploratory lock\n>>      acquisitions that would rather return immediately than have some\n>>      delay.\n>>\n>>   2. The number of tries and sleep time are pulled out of a hat.\n>>\n>>   3. Even with retries, I don't know if you will get the behavior you\n>>      want. The lock procedure for refs is:\n>>\n>>         1. get the lock\n>>         2. check and remember the sha1\n>>         3. release the lock\n>>         4. do some long-running work (like the actual push)\n>>         5. get the lock\n>>         6. check that the sha1 is the same as the remembered one\n>>         7. update the sha1\n>>         8. release the lock\n>>\n>>      Right now you are getting contention on the lock itself. But may\n>>      you not also run afoul of step (6) above? That is, one push updates\n>>      the ref from A to B, then the other one in attempting to go from A\n>>      to B sees that it has already changed to B under our feet and\n>>      complains?\n> \n> Could not anything run afoul of step (6)?  Who knows what might happen in\n> step (4)...\n> \n> However, in my particular case I'm using a \"force\" refspec:\n> \n> \tgit push origin +HEAD:refs/builds/${TAG}\n> \n> so (as Shawn says) step (6) shouldn't matter, right?  Plus, all the\n> concurrent pushes are setting the ref to the same value anyway.\n\nWell, after modifying my build script to ignore failed pushes, I do\noccasionally see failures like this:\n\nremote: fatal: Invalid revision range\n0000000000000000000000000000000000000000..1c58dc4c3fdd9475d26d0eb797cc096fb622a594\nerror: Ref refs/builds/3.3.0-9 is at 1c58dc4c3fdd9475d26d0eb797cc096fb622a594\nbut expected 0000000000000000000000000000000000000000\nremote: error: failed to lock refs/builds/3.3.0-9\n\nSo I guess even the \"force\" refspec is getting blocked by step 6.\n\nFYI, the repo receiving the push is running git 1.7.1.\n\n\t\tM.\n"}]}