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

rlz/mac/lib/rlz_value_store_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "rlz/mac/lib/rlz_value_store_mac.h"
 
+#include "base/eintr_wrapper.h"
 #include "base/mac/foundation_util.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/sys_string_conversions.h"
 #include "rlz/lib/assert.h"
 #include "rlz/lib/lib_values.h"
 #include "rlz/lib/rlz_lib.h"
 
 #import <Foundation/Foundation.h>
 #include <pthread.h>
@@ skipping 211 matching lines @@
   // directory of |lock_file| must exist.
   bool TryGetCrossProcessLock(NSString* lock_filename);
 
   // Releases the lock. Should always be called, even if
   // TryGetCrossProcessLock() returns false.
   void ReleaseLock();
 
   pthread_mutex_t recursive_lock_;
   pthread_t locking_thread_;
 
-  NSDistributedLock* file_lock_;
+  int file_lock_;
 } 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
   // recursive locking with a normal non-recursive mutex.
-  PTHREAD_MUTEX_INITIALIZER
+  PTHREAD_MUTEX_INITIALIZER,
+  0,
+  -1
 };
 
 bool RecursiveCrossProcessLock::TryGetCrossProcessLock(
     NSString* lock_filename) {
   bool just_got_lock = false;
 
   // Emulate a recursive mutex with a non-recursive one.
   if (pthread_mutex_trylock(&recursive_lock_) == EBUSY) {
     if (pthread_equal(pthread_self(), locking_thread_) == 0) {
       // Some other thread has the lock, wait for it.
       pthread_mutex_lock(&recursive_lock_);
       CHECK(locking_thread_ == 0);
       just_got_lock = true;
     }
   } else {
     just_got_lock = true;
   }
 
   locking_thread_ = pthread_self();
 
   // Try to acquire file lock.
   if (just_got_lock) {
     const int kMaxTimeoutMS = 5000;  // Matches windows.
     const int kSleepPerTryMS = 200;
 
-    CHECK(!file_lock_);
-    file_lock_ = [[NSDistributedLock alloc] initWithPath:lock_filename];
+    CHECK(file_lock_ == -1);
+    file_lock_ = open([lock_filename fileSystemRepresentation], O_CREAT, 0755);

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

Use 0666 for the mode argument. There’s no need to set execute bits (0111), but
you can let the umask do its job (don’t be afraid of the 022).

+    if (file_lock_ == -1)
+      return false;
 
-    BOOL got_file_lock = NO;
-    int elapsedMS = 0;
-    while (!(got_file_lock = [file_lock_ tryLock]) &&
-           elapsedMS < kMaxTimeoutMS) {
-      usleep(kSleepPerTryMS * 1000);
-      elapsedMS += kSleepPerTryMS;
+    int flock_result = -1;
+    int elapsed_ms = 0;
+    while ((flock_result =
+               HANDLE_EINTR(flock(file_lock_, LOCK_EX | LOCK_NB))) == -1 &&
+           errno == EWOULDBLOCK &&
+           elapsed_ms < kMaxTimeoutMS) {
+      ignore_result(HANDLE_EINTR(usleep(kSleepPerTryMS * 1000)));

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

Remove HANDLE_EINTR as described. The correct way to implement a timer loop in
light of possible signal interruption is to use nanosleep directly.

+      elapsed_ms += kSleepPerTryMS;
     }
 
-    if (!got_file_lock) {
-      [file_lock_ release];
-      file_lock_ = nil;
+    if (flock_result == -1) {
+      close(file_lock_);
+      file_lock_ = -1;
       return false;
     }
     return true;
   } else {
-    return file_lock_ != nil;
+    return file_lock_ != -1;
   }
 }
 
 void RecursiveCrossProcessLock::ReleaseLock() {
   if (file_lock_) {
-    [file_lock_ unlock];
-    [file_lock_ release];
-    file_lock_ = nil;
+    flock(file_lock_, LOCK_UN);
+    close(file_lock_);

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

You can put your unlock and close in HANDLE_EINTRs too.

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

Done for close. man flock says it can't EINTR (it's supposed to be atomic)

[Mark Mentovai, 2012/08/15 16:10:00]

Nico wrote:
Are you sure about that? The man page isn’t exhaustive. I can show you a flock
that results in EINTR. This doesn’t have anything to do with atomicity. And
you’ve used HANDLE_EINTR on line 280.

[Nico, 2012/08/15 17:02:27]

flock is even explicitly mentioned on the list in
https://groups.google.com/forum/?fromgroups#!msg/chromium-dev/QaNbKkV9c6A/rQl...
, so added a HANDLE_EINTR

+    file_lock_ = -1;
   }
 
   locking_thread_ = 0;
   pthread_mutex_unlock(&recursive_lock_);
 }
 
 
 // This is set during test execution, to write RLZ files into a temporary
 // directory instead of the user's Application Support folder.
 NSString* g_test_folder;
@@ skipping 34 matching lines @@
 // Returns the path of the rlz plist store, also creates the parent directory
 // path if it doesn't exist.
 NSString* RlzPlistFilename() {
   NSString* const kRlzFile = @"RlzStore.plist";
   return [CreateRlzDirectory() stringByAppendingPathComponent:kRlzFile];
 }
 
 // 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];
 }
 
 }  // namespace
 
 ScopedRlzValueStoreLock::ScopedRlzValueStoreLock() {
   bool got_distributed_lock =
       g_recursive_lock.TryGetCrossProcessLock(RlzLockFilename());
   // At this point, we hold the in-process lock, no matter the value of
   // |got_distributed_lock|.
@@ skipping 48 matching lines @@
     g_store_object = NULL;
 
     NSDictionary* dict =
         static_cast<RlzValueStoreMac*>(store_.get())->dictionary();
     VERIFY([dict writeToFile:RlzPlistFilename() atomically:YES]);
   }
 
   // Check that "store_ set" => "file_lock acquired". The converse isn't true,
   // for example if the rlz data file can't be read.
   if (store_.get())
-    CHECK(g_recursive_lock.file_lock_);
-  if (!g_recursive_lock.file_lock_)
+    CHECK(g_recursive_lock.file_lock_ != -1);
+  if (g_recursive_lock.file_lock_ == -1)
     CHECK(!store_.get());
 
   g_recursive_lock.ReleaseLock();
 }
 
 RlzValueStore* ScopedRlzValueStoreLock::GetStore() {
   return store_.get();
 }
 
 namespace testing {
 
 void SetRlzStoreDirectory(const FilePath& directory) {
   base::mac::ScopedNSAutoreleasePool pool;
 
   [g_test_folder release];
   if (directory.empty()) {
     g_test_folder = nil;
   } else {
     // Not Unsafe on OS X.
     g_test_folder =
       [[NSString alloc] initWithUTF8String:directory.AsUTF8Unsafe().c_str()];
   }
 }
 
 }  // namespace testing
 
 }  // namespace rlz_lib