Skip to content

Commit

Permalink
Remove TaskLimiterToken::ReleaseOnce for fix (facebook#8567)
Browse files Browse the repository at this point in the history
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: facebook#8567

Test Plan: watch for more failures in CI

Reviewed By: ajkr

Differential Revision: D29811795

Pulled By: pdillinger

fbshipit-source-id: 135549ebb98fe4f176d1542ed85d5bd6350a40b3
  • Loading branch information
pdillinger authored and facebook-github-bot committed Jul 22, 2021
1 parent 9b41082 commit 84eef26
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 16 deletions.
8 changes: 5 additions & 3 deletions db/db_impl/db_impl_compaction_flush.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand Down
9 changes: 2 additions & 7 deletions util/concurrent_task_limiter_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
7 changes: 1 addition & 6 deletions util/concurrent_task_limiter_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 84eef26

Please sign in to comment.