From ce370d6b952522f7b88910dd5cfea628edc113d9 Mon Sep 17 00:00:00 2001
From: Akanksha Mahajan <akankshamahajan@fb.com>
Date: Fri, 5 Aug 2022 16:23:44 -0700
Subject: [PATCH] Close the Logger before rolling to next one in AutoRollLogger
 (#10488)

Summary:
Close the existing logger first to release the existing
handle before renaming the file using the file system.
Since `AutoRollLogger::Flush` pinned down the `logger_`, `logger_` can't be closed unless its
the last reference otherwise it  gives seg fault during Flush on file
that has been closed.

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

Test Plan: CircleCI jobs

Reviewed By: ajkr

Differential Revision: D38469249

Pulled By: akankshamahajan15

fbshipit-source-id: dfbdb89b4ac37639aefcc503526f24753445fd3f
---
 logging/auto_roll_logger.cc      | 12 ++++++++++++
 logging/auto_roll_logger_test.cc | 11 +++--------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/logging/auto_roll_logger.cc b/logging/auto_roll_logger.cc
index a362b6f23..66f346518 100644
--- a/logging/auto_roll_logger.cc
+++ b/logging/auto_roll_logger.cc
@@ -94,6 +94,18 @@ void AutoRollLogger::RollLogFile() {
       dbname_, now, db_absolute_path_, db_log_dir_);
     now++;
   } while (fs_->FileExists(old_fname, io_options_, &io_context_).ok());
+  // Wait for logger_ reference count to turn to 1 as it might be pinned by
+  // Flush. Pinned Logger can't be closed till Flush is completed on that
+  // Logger.
+  while (logger_.use_count() > 1) {
+  }
+  // Close the existing logger first to release the existing handle
+  // before renaming the file using the file system. If this call
+  // fails there is nothing much we can do and we will continue with the
+  // rename and hence ignoring the result status.
+  if (logger_) {
+    logger_->Close().PermitUncheckedError();
+  }
   Status s = fs_->RenameFile(log_fname_, old_fname, io_options_, &io_context_);
   if (!s.ok()) {
     // What should we do on error?
diff --git a/logging/auto_roll_logger_test.cc b/logging/auto_roll_logger_test.cc
index a3a89ad91..f95f4215d 100644
--- a/logging/auto_roll_logger_test.cc
+++ b/logging/auto_roll_logger_test.cc
@@ -468,18 +468,13 @@ TEST_F(AutoRollLoggerTest, LogFlushWhileRolling) {
   // (1) Need to pin the old logger before beginning the roll, as rolling grabs
   //     the mutex, which would prevent us from accessing the old logger. This
   //     also marks flush_thread with AutoRollLogger::Flush:PinnedLogger.
-  // (2) Need to reset logger during EnvLogger::Flush() to exercise a race
-  //     condition case, which is executing the flush with the pinned (old)
-  //     logger after auto-roll logger has cut over to a new logger.
+  // (2) New logger will be cut in AutoRollLogger::RollLogFile only when flush
+  //     is completed and reference to pinned logger is released.
   // (3) EnvLogger::Flush() happens in both threads but its SyncPoints only
   //     are enabled in flush_thread (the one pinning the old logger).
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependencyAndMarkers(
       {{"AutoRollLogger::Flush:PinnedLogger",
-        "AutoRollLoggerTest::LogFlushWhileRolling:PreRollAndPostThreadInit"},
-       {"EnvLogger::Flush:Begin1",
-        "AutoRollLogger::ResetLogger:BeforeNewLogger"},
-       {"AutoRollLogger::ResetLogger:AfterNewLogger",
-        "EnvLogger::Flush:Begin2"}},
+        "AutoRollLoggerTest::LogFlushWhileRolling:PreRollAndPostThreadInit"}},
       {{"AutoRollLogger::Flush:PinnedLogger", "EnvLogger::Flush:Begin1"},
        {"AutoRollLogger::Flush:PinnedLogger", "EnvLogger::Flush:Begin2"}});
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();