From 84eef260de9c0dc0c7ace8e6f605a2b3efd71038 Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Wed, 21 Jul 2021 17:36:48 -0700
Subject: [PATCH] Remove TaskLimiterToken::ReleaseOnce for fix (#8567)

Summary:
Rare TSAN and valgrind failures are caused by unnecessary
reading of a field on the TaskLimiterToken::limiter_ for an assertion
after the token has been released and the limiter destroyed. To simplify
we can simply destroy the token before triggering DB shutdown
(potentially destroying the limiter). This makes the ReleaseOnce logic
unnecessary.

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

Test Plan: watch for more failures in CI

Reviewed By: ajkr

Differential Revision: D29811795

Pulled By: pdillinger

fbshipit-source-id: 135549ebb98fe4f176d1542ed85d5bd6350a40b3
---
 db/db_impl/db_impl_compaction_flush.cc | 8 +++++---
 util/concurrent_task_limiter_impl.cc   | 9 ++-------
 util/concurrent_task_limiter_impl.h    | 7 +------
 3 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc
index 2b771e360..515bd7422 100644
--- a/db/db_impl/db_impl_compaction_flush.cc
+++ b/db/db_impl/db_impl_compaction_flush.cc
@@ -2785,9 +2785,11 @@ void DBImpl::BackgroundCallCompaction(PrepickedCompaction* prepicked_compaction,
 
     if (prepicked_compaction != nullptr &&
         prepicked_compaction->task_token != nullptr) {
-      // Releasing task tokens affects the DB state, so must be done before we
-      // potentially signal the DB close process to proceed below.
-      prepicked_compaction->task_token->ReleaseOnce();
+      // Releasing task tokens affects (and asserts on) the DB state, so
+      // must be done before we potentially signal the DB close process to
+      // proceed below.
+      prepicked_compaction->task_token.reset();
+      ;
     }
 
     if (made_progress ||
diff --git a/util/concurrent_task_limiter_impl.cc b/util/concurrent_task_limiter_impl.cc
index efa01a17f..2342677d8 100644
--- a/util/concurrent_task_limiter_impl.cc
+++ b/util/concurrent_task_limiter_impl.cc
@@ -59,14 +59,9 @@ ConcurrentTaskLimiter* NewConcurrentTaskLimiter(
   return new ConcurrentTaskLimiterImpl(name, limit);
 }
 
-void TaskLimiterToken::ReleaseOnce() {
-  if (!released_) {
-    --limiter_->outstanding_tasks_;
-    released_ = true;
-  }
+TaskLimiterToken::~TaskLimiterToken() {
+  --limiter_->outstanding_tasks_;
   assert(limiter_->outstanding_tasks_ >= 0);
 }
 
-TaskLimiterToken::~TaskLimiterToken() { ReleaseOnce(); }
-
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/util/concurrent_task_limiter_impl.h b/util/concurrent_task_limiter_impl.h
index 0e6cf1466..d8c1e03cb 100644
--- a/util/concurrent_task_limiter_impl.h
+++ b/util/concurrent_task_limiter_impl.h
@@ -53,16 +53,11 @@ class ConcurrentTaskLimiterImpl : public ConcurrentTaskLimiter {
 class TaskLimiterToken {
  public:
   explicit TaskLimiterToken(ConcurrentTaskLimiterImpl* limiter)
-      : limiter_(limiter), released_(false) {}
+      : limiter_(limiter) {}
   ~TaskLimiterToken();
-  // Releases the token from the `ConcurrentTaskLimiterImpl` if not already
-  // released.
-  // Not thread-safe.
-  void ReleaseOnce();
 
  private:
   ConcurrentTaskLimiterImpl* limiter_;
-  bool released_;
 
   // no copying allowed
   TaskLimiterToken(const TaskLimiterToken&) = delete;