[llvm] r203138 - [Support/LockFileManager] Make the LockFileManager more robust against races.

[Argyrios Kyrtzidis]

On Mar 11, 2014, at 10:13 AM, Kostya Serebryany <kcc at google.com> wrote:

> Can this fix be accompanied with a threaded test? 

This may or may not be more work than the fix itself, could you file a bugzilla request ?

> 
> 
> On Tue, Mar 11, 2014 at 8:44 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 
> On Mar 11, 2014, at 7:40 AM, Kostya Serebryany <kcc at google.com> wrote:
> 
>> a question here: if this was a race, it could have been found with tsan (this or a similar one *was* caught by tsan). 
>> Do we have enough tests for multi-threaded code to run a tsan bootstrap bot? 
> 
> I don’t think so, unfortunately.
> 
>> 
>> --kcc 
>> 
>> 
>> On Thu, Mar 6, 2014 at 9:37 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> Author: akirtzidis
>> Date: Thu Mar  6 11:37:10 2014
>> New Revision: 203138
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=203138&view=rev
>> Log:
>> [Support/LockFileManager] Make the LockFileManager more robust against races.
>> 
>> There was a race where:
>> - The LockFileManager tries to own the lock file and fails.
>> - The other owner then releases and removes the lock file.
>> - The LockFileManager tries to read the owner info from the lock file but fails now.
>> 
>> In such a case have LockFileManager try to get ownership again, instead of error'ing out.
>> 
>> Modified:
>>     llvm/trunk/lib/Support/LockFileManager.cpp
>> 
>> Modified: llvm/trunk/lib/Support/LockFileManager.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/LockFileManager.cpp?rev=203138&r1=203137&r2=203138&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Support/LockFileManager.cpp (original)
>> +++ llvm/trunk/lib/Support/LockFileManager.cpp Thu Mar  6 11:37:10 2014
>> @@ -115,25 +115,42 @@ LockFileManager::LockFileManager(StringR
>>      }
>>    }
>> 
>> -  // Create a symbolic link from the lock file name. If this succeeds, we're done.
>> -  // Note that we are using symbolic link because hard links are not supported
>> -  // by all filesystems.
>> -  error_code EC
>> -    = sys::fs::create_symbolic_link(UniqueLockFileName.str(),
>> -                                      LockFileName.str());
>> -  if (EC == errc::success)
>> -    return;
>> -
>> -  // Someone else managed to create the lock file first. Wipe out our unique
>> -  // lock file (it's useless now) and read the process ID from the lock file.
>> -  sys::fs::remove(UniqueLockFileName.str());
>> -  if ((Owner = readLockFile(LockFileName)))
>> -    return;
>> -
>> -  // There is a lock file that nobody owns; try to clean it up and report
>> -  // an error.
>> -  sys::fs::remove(LockFileName.str());
>> -  Error = EC;
>> +  while (1) {
>> +    // Create a symbolic link from the lock file name. If this succeeds, we're
>> +    // done. Note that we are using symbolic link because hard links are not
>> +    // supported by all filesystems.
>> +    error_code EC
>> +      = sys::fs::create_symbolic_link(UniqueLockFileName.str(),
>> +                                        LockFileName.str());
>> +    if (EC == errc::success)
>> +      return;
>> +
>> +    if (EC != errc::file_exists) {
>> +      Error = EC;
>> +      return;
>> +    }
>> +
>> +    // Someone else managed to create the lock file first. Read the process ID
>> +    // from the lock file.
>> +    if ((Owner = readLockFile(LockFileName))) {
>> +      // Wipe out our unique lock file (it's useless now)
>> +      sys::fs::remove(UniqueLockFileName.str());
>> +      return;
>> +    }
>> +
>> +    if (!sys::fs::exists(LockFileName.str())) {
>> +      // The previous owner released the lock file before we could read it.
>> +      // Try to get ownership again.
>> +      continue;
>> +    }
>> +
>> +    // There is a lock file that nobody owns; try to clean it up and get
>> +    // ownership.
>> +    if ((EC = sys::fs::remove(LockFileName.str()))) {
>> +      Error = EC;
>> +      return;
>> +    }
>> +  }
>>  }
>> 
>>  LockFileManager::LockFileState LockFileManager::getState() const {
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140311/649d37ac/attachment.html>