rlz/mac: Make sure a crashing process doesn't leave a stale lockfile behind.

rlz/mac/lib/rlz_value_store_mac.mm

Index: rlz/mac/lib/rlz_value_store_mac.mm
diff --git a/rlz/mac/lib/rlz_value_store_mac.mm b/rlz/mac/lib/rlz_value_store_mac.mm
index 5313a3013a9b80a016434799a01b475891bb8877..6ac38923e173ac6dbac81826f13bfa291b6fd1c2 100644
--- a/rlz/mac/lib/rlz_value_store_mac.mm
+++ b/rlz/mac/lib/rlz_value_store_mac.mm
@@ -235,7 +235,7 @@ struct RecursiveCrossProcessLock {
   pthread_mutex_t recursive_lock_;
   pthread_t locking_thread_;
 
-  NSDistributedLock* file_lock_;
+  int file_lock_;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

ScopedFD?

[Nico, 2012/08/15 03:24:12]

It needs to be closed while the mutex is still held.

[Scott Hess - ex-Googler, 2012/08/15 19:55:04]

That's what reset() is for.

 } g_recursive_lock = {
   // PTHREAD_RECURSIVE_MUTEX_INITIALIZER doesn't exist before 10.7 and is buggy
   // on 10.7 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51906#c34), so emulate
@@ -267,32 +267,42 @@ bool RecursiveCrossProcessLock::TryGetCrossProcessLock(
     const int kSleepPerTryMS = 200;
 
     CHECK(!file_lock_);
-    file_lock_ = [[NSDistributedLock alloc] initWithPath:lock_filename];
+    file_lock_ = open([lock_filename fileSystemRepresentation], O_CREAT);
+    if (file_lock_ == -1) {
+      file_lock_ = 0;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

0 is a valid fd.  Suggest using -1 for invalid, just to be safe.

[Mark Mentovai, 2012/08/15 00:50:54]

But 0 is a valid FD. You should rewrite things to use -1 as the invalid lock
value (your initializer on lines 239-244, your CHECK on line 269, your reset on
line 285, your test on line 297, etc.)

[Nico, 2012/08/15 03:24:12]

Done. (But in practice 0 is stdin, so in practice this should've been fine as
is, right?)

[Mark Mentovai, 2012/08/15 13:36:28]

Nico wrote:
You can close stdin.

+      return false;
+    }
 
-    BOOL got_file_lock = NO;
+    int flock_result = -1;
     int elapsedMS = 0;

[Mark Mentovai, 2012/08/15 00:50:54]

elapsed_ms

[Nico, 2012/08/15 03:24:12]

Done.

-    while (!(got_file_lock = [file_lock_ tryLock]) &&
+    while ((flock_result = flock(file_lock_, LOCK_EX | LOCK_NB)) == -1 &&

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

errno == EWOULDBLOCK &&

Then you can lift the if() clause and drop the else clause because it's already
handled outside the loop.

[Mark Mentovai, 2012/08/15 00:50:54]

HANDLE_EINTR

[Nico, 2012/08/15 03:24:12]

Done.

            elapsedMS < kMaxTimeoutMS) {
-      usleep(kSleepPerTryMS * 1000);
-      elapsedMS += kSleepPerTryMS;
+      if (errno == EWOULDBLOCK) {
+        usleep(kSleepPerTryMS * 1000);
+        elapsedMS += kSleepPerTryMS;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

usleep() could be interrupted by a signal, in which case elapsedMS would be
bogus.  Admittedly at 200ms per shot, not super bogus, but still.  Can you use
TimeTicks() or something or the sort to drive this?  Though I guess it's just as
good as before.

Also, how much contention could there be?  As implemented, someone could get
denied the lock while someone else jumps in after and claims it.  Though, again,
just as good as before.

[Nico, 2012/08/15 03:24:12]

Added HANDLE_EINTR here too.
In practice, chrome is the only app that uses rlz. (In theory, both chrome and
canary could write to it, but canary is always unbranded and shouldn't use rlz),
so I think that's fine as is.

(Out of interest: What's a better way to do this, should there be contention?)

[Mark Mentovai, 2012/08/15 13:36:28]

HANDLE_EINTR doesn’t really help here because the goal is to get a timed sleep.
In fact, HANDLE_EINTR may hurt because it will cause any interrupted sleep to be
restarted for the full kSleepPerTryMS milliseconds, and in a pathological
“signal storm,” you’ll never leave HANDLE_EINTR’s loop around usleep.

[Nico, 2012/08/15 14:52:50]

Good (pathological) point! Removed it again.

[Scott Hess - ex-Googler, 2012/08/15 19:55:04]

I think the POSIXy way is something like:

  int end_time = time() + kTimeout;
  do {
    alarm(end_time - time());
    rc = flock(fd_, LOCK_EX);
  } while (rc == -1 && errno == EINTR && time() < end_time);
    
Of course, it needs to be rewritten with gettimeofday() and setitimer() to give
better resolution, and someone would need to verify whether this works right
with threading.

Aaaaaand ... I guess it's still open for contention if you have other signals in
the system.  But if you're lucky enough not to have other signals, and not to
need to re-lock while a previous lock's alarm is still pending, then you'll just
wait until either you get the lock or the alarm fires.  So another process
couldn't snipe you.

IOW, let's not go there.

[Mark Mentovai, 2012/08/15 20:18:39]

shess wrote:
Even without other signals, there’s no explicit guarantee that the process that
ultimately wins the exclusive lock will be the first process to request it that
blocked, so you’d be relying on an implementation detail (that incidentally is
shared by the Mac and Linux kernels).

+      } else {
+        close(file_lock_);
+        file_lock_ = 0;
+        return false;
+      }
     }
 
-    if (!got_file_lock) {
-      [file_lock_ release];
-      file_lock_ = nil;
+    if (flock_result == -1) {
+      close(file_lock_);

[Mark Mentovai, 2012/08/15 00:50:54]

This duplicates what’s in the loop. I think you only need this copy.

[Nico, 2012/08/15 03:24:12]

Done, together with shess's suggestion above.

+      file_lock_ = 0;
       return false;
     }
     return true;
   } else {
-    return file_lock_ != nil;
+    return file_lock_ != 0;
   }
 }
 
 void RecursiveCrossProcessLock::ReleaseLock() {
   if (file_lock_) {
-    [file_lock_ unlock];
-    [file_lock_ release];
-    file_lock_ = nil;
+    flock(file_lock_, LOCK_UN);
+    close(file_lock_);
+    file_lock_ = 0;
   }
 
   locking_thread_ = 0;
@@ -347,7 +357,7 @@ NSString* RlzPlistFilename() {
 // Returns the path of the rlz lock file, also creates the parent directory
 // path if it doesn't exist.
 NSString* RlzLockFilename() {
-  NSString* const kRlzFile = @"lockfile";
+  NSString* const kRlzFile = @"flockfile";
   return [CreateRlzDirectory() stringByAppendingPathComponent:kRlzFile];
 }