Deflake unit test BackupEngineTest.Concurrency (#10069)

Summary:
After https://github.com/facebook/rocksdb/issues/9984, BackupEngineTest.Concurrency becomes flaky.

During DB::Open(), someone else can rename/remove the LOG file, causing
this thread's `CreateLoggerFromOptions()` to fail. The reason is that the operation sequence
of "FileExists -> Rename" is not atomic. It's possible that a FileExists() returns OK, but the file
gets deleted before Rename(), causing the latter to return IOError with PathNotFound subcode.

Although it's not encouraged to concurrently modify the contents of the directories managed by
the database instance in this case, we can still perform some simple handling to make DB::Open()
more robust. In this case, we can check if a racing thread has deleted the original LOG file, we can
allow this thread to continue creating a new LOG file.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10069

Test Plan: ~/gtest-parallel/gtest-parallel -r 100 ./backup_engine_test --gtest_filter=BackupEngineTest.Concurrency

Reviewed By: jay-zhuang

Differential Revision: D36736913

Pulled By: riversand963

fbshipit-source-id: 3cbe92d77ca175e55e586bdb1a32ac8107217ae6

diff --git a/logging/auto_roll_logger.cc b/logging/auto_roll_logger.cc
index f7929434e582c47f7590eb6b96d35731fe12bde3..cfc8d43713d1c552d3a2552934c07af882788665 100644 (file)
--- a/logging/auto_roll_logger.cc
+++ b/logging/auto_roll_logger.cc
@@ -312,6 +312,28 @@ Status CreateLoggerFromOptions(const std::string& dbname,
     s = env->RenameFile(
         fname, OldInfoLogFileName(dbname, clock->NowMicros(), db_absolute_path,
                                   options.db_log_dir));
+
+    // The operation sequence of "FileExists -> Rename" is not atomic. It's
+    // possible that FileExists returns OK but file gets deleted before Rename.
+    // This can cause Rename to return IOError with subcode PathNotFound.
+    // Although it may be a rare case and applications should be discouraged
+    // to not concurrently modifying the contents of the directories accessed
+    // by the database instance, it is still helpful if we can perform some
+    // simple handling of this case. Therefore, we do the following:
+    // 1. if Rename() returns IOError with PathNotFound subcode, then we check
+    //    whether the source file, i.e. LOG, exists.
+    // 2. if LOG exists, it means Rename() failed due to something else. Then
+    //    we report error.
+    // 3. if LOG does not exist, it means it may have been removed/renamed by
+    //    someone else. Since it does not exist, we can reset Status to OK so
+    //    that this caller can try creating a new LOG file. If this succeeds,
+    //    we should still allow it.
+    if (s.IsPathNotFound()) {
+      s = env->FileExists(fname);
+      if (s.IsNotFound()) {
+        s = Status::OK();
+      }
+    }
   } else if (s.IsNotFound()) {
     // "LOG" is not required to exist since this could be a new DB.
     s = Status::OK();