mvcc: some locking cleanup

Close was notifying waiters before erasing them from the waiters_ list. In
practice this didn't matter: lock_ was always held around any manipulation
of waiters_. But for consistency, let's do what AdjustCleanTime does.

AdjustCleanTime required lock_ to be held, so let's enforce that.

Change-Id: I189553d0f589794ad830a30b3c0b4ce5fd5569ba
Reviewed-on: http://gerrit.cloudera.org:8080/14816
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <[email protected]>

src/kudu/tablet/mvcc.cc

@@ -142,7 +142,7 @@ void MvccManager::CommitTransaction(Timestamp timestamp) {
   if (was_earliest && safe_time_ >= timestamp) {
     // If this transaction was the earliest in-flight, we might have to adjust
     // the "clean" timestamp.
-    AdjustCleanTime();
+    AdjustCleanTimeUnlocked();
   }
 }
 
@@ -206,7 +206,7 @@ void MvccManager::AdjustSafeTime(Timestamp safe_time) {
     return;
   }
 
-  AdjustCleanTime();
+  AdjustCleanTimeUnlocked();
 }
 
 // Remove any elements from 'v' which are < the given watermark.
@@ -226,12 +226,15 @@ void MvccManager::Close() {
   std::lock_guard<LockType> l(lock_);
   auto iter = waiters_.begin();
   while (iter != waiters_.end()) {
-    (*iter)->latch->CountDown();
+    auto* waiter = *iter;
     iter = waiters_.erase(iter);
+    waiter->latch->CountDown();
   }
 }
 
-void MvccManager::AdjustCleanTime() {
+void MvccManager::AdjustCleanTimeUnlocked() {
+  DCHECK(lock_.is_locked());
+
   // There are two possibilities:
   //
   // 1) We still have an in-flight transaction earlier than 'safe_time_'.
@@ -269,7 +272,7 @@ void MvccManager::AdjustCleanTime() {
   if (PREDICT_FALSE(!waiters_.empty())) {
     auto iter = waiters_.begin();
     while (iter != waiters_.end()) {
-      WaitingState* waiter = *iter;
+      auto* waiter = *iter;
       if (IsDoneWaitingUnlocked(*waiter)) {
         iter = waiters_.erase(iter);
         waiter->latch->CountDown();
@@ -305,8 +308,8 @@ Status MvccManager::WaitUntil(WaitFor wait_for, Timestamp ts, const MonoTime& de
   // We timed out. We need to clean up our entry in the waiters_ array.
 
   std::lock_guard<LockType> l(lock_);
-  // It's possible that while we were re-acquiring the lock, we did not get
-  // notified. In that case, we have no cleanup to do.
+  // It's possible that we got notified while we were re-acquiring the lock. In
+  // that case, we have no cleanup to do.
   if (waiting_state.latch->count() == 0) {
     return CheckOpen();
   }

src/kudu/tablet/mvcc.h

@@ -354,7 +354,9 @@ class MvccManager {
   // Adjusts the clean time, i.e. the timestamp such that all transactions with
   // lower timestamps are committed or aborted, based on which transactions are
   // currently in flight and on what is the latest value of 'safe_time_'.
-  void AdjustCleanTime();
+  //
+  // Must be called with lock_ held.
+  void AdjustCleanTimeUnlocked();
 
   // Advances the earliest in-flight timestamp, based on which transactions are
   // currently in-flight. Usually called when the previous earliest transaction