Make TryGetLock throw exceptions instead of returning false on failure.

This allows us to give much more detailed reasons about why we failed to get a lock. The exception for an expected failure (someone else is holding the lock) should always be a BackupStoreException::CouldNotLockStoreAccount.
boxbackup · Jan 12, 2016 · 6e610e7 · 6e610e7
1 parent 8589bd9
commit 6e610e7
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 34 deletions.
diff --git a/lib/backupstore/BackupFileSystem.cpp b/lib/backupstore/BackupFileSystem.cpp
@@ -38,37 +38,47 @@
 
 void BackupFileSystem::GetLock()
 {
-	bool gotLock = false;
-	int triesLeft = 8;
-	do
+	for(int triesLeft = 8; triesLeft >= 0; triesLeft--)
 	{
-		gotLock = TryGetLock();
-		if(!gotLock)
+		try
 		{
-			--triesLeft;
-			::sleep(1);
+			TryGetLock();
+		}
+		catch(BackupStoreException &e)
+		{
+			if(EXCEPTION_IS_TYPE(e, BackupStoreException,
+				CouldNotLockStoreAccount) && triesLeft)
+			{
+				// Try again as long as we have retries left.
+			}
+			else
+			{
+				throw;
+			}
 		}
 	}
-	while (!gotLock && triesLeft > 0);
+}
 
-	if (!gotLock)
+void RaidBackupFileSystem::TryGetLock()
+{
+	if(mWriteLock.GotLock())
 	{
-		THROW_EXCEPTION_MESSAGE(BackupStoreException,
-			CouldNotLockStoreAccount, "Failed to get exclusive "
-			"lock on account");
+		return;
 	}
-}
 
-bool RaidBackupFileSystem::TryGetLock()
-{
 	// Make the filename of the write lock file
 	std::string writeLockFile;
 	StoreStructure::MakeWriteLockFilename(mAccountRootDir, mStoreDiscSet,
 		writeLockFile);
 
 	// Request the lock
-	return mWriteLock.TryAndGetLock(writeLockFile.c_str(),
-		0600 /* restrictive file permissions */);
+	if(!mWriteLock.TryAndGetLock(writeLockFile.c_str(),
+		0600 /* restrictive file permissions */))
+	{
+		THROW_EXCEPTION_MESSAGE(BackupStoreException,
+			CouldNotLockStoreAccount, "Failed to get exclusive "
+			"lock on account");
+	}
 }
 
 std::string RaidBackupFileSystem::GetObjectFileName(int64_t ObjectID,

diff --git a/lib/backupstore/BackupFileSystem.h b/lib/backupstore/BackupFileSystem.h
@@ -13,6 +13,7 @@
 
 #include <string>
 
+#include "autogen_BackupStoreException.h"
 #include "HTTPResponse.h"
 #include "NamedLock.h"
 #include "S3Client.h"
@@ -49,7 +50,7 @@ class BackupFileSystem
 
 	BackupFileSystem() { }
 	virtual ~BackupFileSystem() { }
-	virtual bool TryGetLock() = 0;
+	virtual void TryGetLock() = 0;
 	virtual void GetLock();
 	virtual void ReleaseLock() = 0;
 	virtual int GetBlockSize() = 0;
@@ -86,7 +87,7 @@ class RaidBackupFileSystem : public BackupFileSystem
 	  mAccountRootDir(AccountRootDir),
 	  mStoreDiscSet(discSet)
 	{ }
-	virtual bool TryGetLock();
+	virtual void TryGetLock();
 	virtual void ReleaseLock()
 	{
 		mWriteLock.ReleaseLock();
@@ -135,7 +136,7 @@ class S3BackupFileSystem : public BackupFileSystem
 	  mBasePath(BasePath),
 	  mrClient(rClient)
 	{ }
-	virtual bool TryGetLock() { return false; }
+	virtual void TryGetLock() { THROW_EXCEPTION(BackupStoreException, CouldNotLockStoreAccount); }
 	virtual void ReleaseLock() { }
 	virtual int GetBlockSize();
 	virtual std::auto_ptr<BackupStoreInfo> GetBackupStoreInfo(int32_t AccountID,

diff --git a/lib/backupstore/BackupStoreContext.cpp b/lib/backupstore/BackupStoreContext.cpp
@@ -187,25 +187,57 @@ bool BackupStoreContext::AttemptToGetWriteLock()
 	CHECK_FILESYSTEM_INITIALISED();
 
 	// Request the lock
-	bool gotLock = mpFileSystem->TryGetLock();
+	bool gotLock = false;
 
-	if(!gotLock && mpHousekeeping)
+	try
 	{
-		// The housekeeping process might have the thing open -- ask it to stop
-		char msg[256];
-		int msgLen = snprintf(msg, sizeof(msg), "r%x\n", mClientID);
-		// Send message
-		mpHousekeeping->SendMessageToHousekeepingProcess(msg, msgLen);
+		mpFileSystem->TryGetLock();
+		// If we got to here, then it worked!
+		gotLock = true;
+	}
+	catch(BackupStoreException &e)
+	{
+		if(!EXCEPTION_IS_TYPE(e, BackupStoreException, CouldNotLockStoreAccount))
+		{
+			// We don't know what this error is.
+			throw;
+		}
 
-		// Then try again a few times
-		int tries = MAX_WAIT_FOR_HOUSEKEEPING_TO_RELEASE_ACCOUNT;
-		do
+		if(mpHousekeeping)
 		{
-			::sleep(1 /* second */);
-			--tries;
-			gotLock = mpFileSystem->TryGetLock();
+			// The housekeeping process might have the thing open -- ask it to stop
+			char msg[256];
+			int msgLen = snprintf(msg, sizeof(msg), "r%x\n", mClientID);
 
-		} while(!gotLock && tries > 0);
+			// Send message
+			mpHousekeeping->SendMessageToHousekeepingProcess(msg, msgLen);
+
+			// Then try again a few times
+			for(int tries = MAX_WAIT_FOR_HOUSEKEEPING_TO_RELEASE_ACCOUNT;
+				tries >= 0; tries--)
+			{
+				try
+				{
+					::sleep(1 /* second */);
+					mpFileSystem->TryGetLock();
+					// If we got to here, then it worked!
+					gotLock = true;
+					break;
+				}
+				catch(BackupStoreException &e)
+				{
+					if(EXCEPTION_IS_TYPE(e, BackupStoreException,
+						CouldNotLockStoreAccount))
+					{
+						// keep trying
+					}
+					else
+					{
+						throw;
+					}
+				}
+			}
+		}
 	}
 
 	if(gotLock)