cc core: Deadlock detector API refactoring

Implements new dependency tracking API for Deadlock detector. It clearly separates resource ownership and resource awaiting tracking.

Adds a few more deadlock detector tests.
commit_hash:3f99fd4a2a6ecd2b500dfec44e2f3284aca0e939

Author: lexeyo

core/src/engine/deadlock_detector.cpp

@@ -54,6 +54,8 @@ class CycleDetector final {
               cycle(std::move(cycle))
         {}
 
+        // Actors dependency cycle. Each actor (except the last) blocks the next one.
+        // The last one blocks the first.
         std::vector<Vertex> cycle;
     };
 
@@ -121,27 +123,26 @@ StateBase::StateBase(DeadlockDetector dd) {
 
 StateBase::~StateBase() = default;
 
-void StateBase::HookBeforeAddDependency(const Actor& subject, const Actor& object) {
+void StateBase::AddDependency(const Actor& from, const Actor& to) {
     if (!impl_->enabled) {
         return;
     }
 
     const auto* current = current_task::GetCurrentTaskContextUnchecked();
-    if (current && current == &subject && impl_->collect_stacktrace) {
+    if (current && current == &from && impl_->collect_stacktrace) {
         auto bt = impl_->backtraces.Lock();
         bt->emplace(current, boost::stacktrace::stacktrace());
     }
 
     auto edges = impl_->active_dependencies.Lock();
-    auto& subject_dependencies = (*edges)[&subject];
+    auto& dependencies = (*edges)[&from];
     UASSERT_MSG(
-        std::find(subject_dependencies.begin(), subject_dependencies.end(), &object) == subject_dependencies.end(),
-        fmt::format("Adding already existing dependency {} -> {}", ToAssertString(subject), ToAssertString(object))
+        std::find(dependencies.begin(), dependencies.end(), &to) == dependencies.end(),
+        fmt::format("Adding already existing dependency {} -> {}", ToAssertString(from), ToAssertString(to))
     );
-    subject_dependencies.emplace_back(&object);
-
+    dependencies.emplace_back(&to);
     CycleDetector cd(*edges);
-    auto cycle = cd.FindCycle(&object);
+    auto cycle = cd.FindCycle(&to);
     if (cycle) {
         auto bt = impl_->backtraces.Lock();
         for (const auto& actor : *cycle) {
@@ -156,35 +157,50 @@ void StateBase::HookBeforeAddDependency(const Actor& subject, const Actor& objec
     }
 }
 
-void StateBase::HookBeforeRemoveDependency(const Actor& subject, const Actor& object) noexcept {
+void StateBase::RemoveDependency(const Actor& from, const Actor& to) noexcept {
     if (!impl_->enabled) {
         return;
     }
 
     auto edges = impl_->active_dependencies.Lock();
-    auto& v = (*edges)[&subject];
+    auto& v = (*edges)[&from];
 
-    auto it = std::find(v.begin(), v.end(), &object);
+    auto it = std::find(v.begin(), v.end(), &to);
     if (it == v.end()) {
-        utils::AbortWithStacktrace(fmt::format(
-            "Trying to stop waiting while not waiting! {} => {}",
-            ToAssertString(subject),
-            ToAssertString(object)
-        ));
+        utils::AbortWithStacktrace(
+            fmt::format("Trying to stop waiting while not waiting! {} => {}", ToAssertString(from), ToAssertString(to))
+        );
     }
     v.erase(it);
     if (v.empty()) {
-        edges->erase(&subject);
+        edges->erase(&from);
     }
 }
 
-void StateBase::HookActorDestroy(const Actor& object) {
+void StateBase::OnResourceAcquire(const Actor& owner, const Actor& resource) {
+    // Resource release is dependent on the owner
+    AddDependency(resource, owner);
+}
+
+void StateBase::OnResourceRelease(const Actor& owner, const Actor& resource) noexcept {
+    RemoveDependency(resource, owner);
+}
+
+void StateBase::OnWaitForResourceStart(const Actor& waiting, const Actor& resource) {
+    AddDependency(waiting, resource);
+}
+
+void StateBase::OnWaitForResourceFinish(const Actor& waiting, const Actor& resource) noexcept {
+    RemoveDependency(waiting, resource);
+}
+
+void StateBase::OnActorDestroy(const Actor& actor) {
     auto edges = impl_->active_dependencies.Lock();
-    auto it = edges->find(&object);
+    auto it = edges->find(&actor);
     if (it != edges->end() && !it->second.empty()) {
         utils::AbortWithStacktrace(fmt::format(
             "Trying to destroy {} while still waiting for {}!",
-            ToAssertString(object),
+            ToAssertString(actor),
             ToAssertString(*it->second.front())
         ));
     }
@@ -204,16 +220,16 @@ State& GetState() {
     return pool.GetDeadlockDetectorState();
 }
 
-WaitScope::WaitScope(const Actor& a)
-    : actor_(a)
+WaitScope::WaitScope(const Actor& resource)
+    : resource_(resource)
 {
     auto& dd_state = GetState();
-    dd_state.HookBeforeAddDependency(current_task::GetCurrentTaskContext(), actor_);
+    dd_state.OnWaitForResourceStart(current_task::GetCurrentTaskContext(), resource_);
 }
 
 WaitScope::~WaitScope() {
     auto& dd_state = GetState();
-    dd_state.HookBeforeRemoveDependency(current_task::GetCurrentTaskContext(), actor_);
+    dd_state.OnWaitForResourceFinish(current_task::GetCurrentTaskContext(), resource_);
 }
 
 }  // namespace engine::deadlock_detector

core/src/engine/deadlock_detector.hpp

@@ -18,16 +18,24 @@ class StateBase {
 
     virtual ~StateBase();
 
-    void HookBeforeAddDependency(const Actor& subject, const Actor& object);
+    void OnResourceAcquire(const Actor& owner, const Actor& resource);
 
-    void HookBeforeRemoveDependency(const Actor& subject, const Actor& object) noexcept;
+    void OnResourceRelease(const Actor& owner, const Actor& resource) noexcept;
 
-    void HookActorDestroy(const Actor& object);
+    void OnWaitForResourceStart(const Actor& waiting, const Actor& resource);
+
+    void OnWaitForResourceFinish(const Actor& waiting, const Actor& resource) noexcept;
+
+    void OnActorDestroy(const Actor& actor);
 
 protected:
     virtual void OnCycleFound(const std::vector<const Actor*>& cycle) = 0;
 
 private:
+    void AddDependency(const Actor& from, const Actor& to);
+
+    void RemoveDependency(const Actor& from, const Actor& to) noexcept;
+
     struct Impl;
     utils::FastPimpl<Impl, 232, 8> impl_;
 };
@@ -45,12 +53,12 @@ State& GetState();
 
 class WaitScope final {
 public:
-    explicit WaitScope(const Actor& a);
+    explicit WaitScope(const Actor& resource);
     WaitScope(WaitScope&&) = delete;
     ~WaitScope();
 
 private:
-    const Actor& actor_;
+    const Actor& resource_;
 };
 
 }  // namespace engine::deadlock_detector

core/src/engine/deadlock_detector_test.cpp

@@ -67,45 +67,76 @@ UTEST(DeadlockDetector, Ctr) {
 
 UTEST(DeadlockDetector, NoCycle) {
     MockState state;
-    MockActor a1;
-    MockActor a2;
+    MockActor actor;
+    MockActor resource;
 
-    state.HookBeforeAddDependency(a1, a2);
-    state.HookBeforeRemoveDependency(a1, a2);
+    state.OnResourceAcquire(actor, resource);
+    state.OnResourceRelease(actor, resource);
+
+    state.OnWaitForResourceStart(actor, resource);
+    state.OnWaitForResourceFinish(actor, resource);
 
     SUCCEED();
 }
 
-UTEST(DeadlockDetector, Cycle2) {
+UTEST(DeadlockDetector, Cycle2AcquireWait) {
     MockState state;
-    MockActor a1;
-    MockActor a2;
+    MockActor actor;
+    MockActor resource;
 
-    state.HookBeforeAddDependency(a1, a2);
-    EXPECT_THROW_CYCLE(state.HookBeforeAddDependency(a2, a1), &a1, &a2);
+    // actor -> resource -> actor
+    state.OnResourceAcquire(actor, resource);
+    EXPECT_THROW_CYCLE(state.OnWaitForResourceStart(actor, resource), &resource, &actor);
 }
 
-UTEST(DeadlockDetector, Cycle3) {
+UTEST(DeadlockDetector, Cycle2WaitAcquire) {
     MockState state;
-    MockActor a1;
-    MockActor a2;
-    MockActor a3;
+    MockActor actor;
+    MockActor resource;
 
-    state.HookBeforeAddDependency(a1, a2);
-    state.HookBeforeAddDependency(a2, a3);
-    EXPECT_THROW_CYCLE(state.HookBeforeAddDependency(a3, a1), &a1, &a3, &a2);
+    // resource -> actor -> resource
+    state.OnWaitForResourceStart(actor, resource);
+    EXPECT_THROW_CYCLE(state.OnResourceAcquire(actor, resource), &actor, &resource);
+}
+
+UTEST(DeadlockDetector, Cycle3) {
+    MockState state;
+    MockActor actor1;
+    MockActor actor2;
+    MockActor resource;
+
+    // resource -> actor1 -> actor2 -> resource
+    state.OnResourceAcquire(actor1, resource);
+    state.OnWaitForResourceStart(actor2, resource);
+    EXPECT_THROW_CYCLE(state.OnWaitForResourceStart(actor1, actor2), &actor2, &actor1, &resource);
 }
 
 UTEST(DeadlockDetector, RightCycle) {
     MockState state;
-    MockActor a1;
-    MockActor a2;
-    MockActor b1;
-    MockActor b2;
-
-    state.HookBeforeAddDependency(a1, a2);
-    state.HookBeforeAddDependency(b1, b2);
-    EXPECT_THROW_CYCLE(state.HookBeforeAddDependency(a2, a1), &a1, &a2);
+    MockActor actor1;
+    MockActor actor2;
+    MockActor resource1;
+    MockActor resource2;
+
+    // actor1 -> resource1 -> actor1
+    // resource2 -> actor2
+    state.OnResourceAcquire(actor1, resource1);
+    state.OnResourceAcquire(actor2, resource2);
+    EXPECT_THROW_CYCLE(state.OnWaitForResourceStart(actor1, resource1), &resource1, &actor1);
+}
+
+UTEST(DeadlockDetector, ClassicMutualDeadlock) {
+    MockState state;
+    MockActor actor1;
+    MockActor actor2;
+    MockActor resource1;
+    MockActor resource2;
+
+    // actor1 -> resource2 -> actor2 -> resource1 -> actor1
+    state.OnResourceAcquire(actor1, resource1);
+    state.OnResourceAcquire(actor2, resource2);
+    state.OnWaitForResourceStart(actor1, resource2);
+    EXPECT_THROW_CYCLE(state.OnWaitForResourceStart(actor2, resource1), &resource1, &actor2, &resource2, &actor1);
 }
 
 UTEST(DeadlockDetector, NoCycleOnDiamond) {
@@ -130,12 +161,12 @@ UTEST(DeadlockDetector, NoCycleOnDiamond) {
     MockActor v3;
     MockActor v4;
 
-    state.HookBeforeAddDependency(v0, v1);
-    state.HookBeforeAddDependency(v0, v4);
-    state.HookBeforeAddDependency(v1, v2);
-    state.HookBeforeAddDependency(v4, v2);
-    state.HookBeforeAddDependency(v2, v3);
-    state.HookBeforeAddDependency(start, v0);
+    state.OnWaitForResourceStart(v0, v1);
+    state.OnWaitForResourceStart(v0, v4);
+    state.OnWaitForResourceStart(v1, v2);
+    state.OnWaitForResourceStart(v4, v2);
+    state.OnWaitForResourceStart(v2, v3);
+    state.OnWaitForResourceStart(start, v0);
 
     SUCCEED();
 }

core/src/engine/impl/mutex_impl.hpp

@@ -163,7 +163,7 @@ void MutexImpl<Waiters>::lock() {
 template <class Waiters>
 void MutexImpl<Waiters>::unlock() {
     auto& dd_state = deadlock_detector::GetState();
-    dd_state.HookBeforeRemoveDependency(*this, current_task::GetCurrentTaskContext());
+    dd_state.OnResourceRelease(current_task::GetCurrentTaskContext(), *this);
 
 #if USERVER_IMPL_HAS_TSAN
     __tsan_mutex_pre_unlock(this, 0);
@@ -200,7 +200,7 @@ bool MutexImpl<Waiters>::try_lock() {
 
     auto& dd_state = deadlock_detector::GetState();
     if (result) {
-        dd_state.HookBeforeAddDependency(*this, current_task::GetCurrentTaskContext());
+        dd_state.OnResourceAcquire(current_task::GetCurrentTaskContext(), *this);
     }
 
     return result;
@@ -236,7 +236,7 @@ bool MutexImpl<Waiters>::try_lock_until(Deadline deadline) {
 
     auto& dd_state = deadlock_detector::GetState();
     if (result) {
-        dd_state.HookBeforeAddDependency(*this, current_task::GetCurrentTaskContext());
+        dd_state.OnResourceAcquire(current_task::GetCurrentTaskContext(), *this);
     }
 
     return result;