bug dynamic-configs: fix deadlock

commit_hash:f1aa24a0663877e2f846af376abb98ae54247a56

Author: segoon

core/src/dynamic_config/config_test.cpp

@@ -545,4 +545,32 @@ UTEST(DynamicConfig, DeadlockOnSubscribeInSendEvent) {
     subscriber.cb = {};  // cleanup to avoid UAF in dtr (in debug)
 }
 
+UTEST(DynamicConfig, DeadlockOnSubscribeInSendEventDiff) {
+    struct LocalSubscriber {
+        void OnDiffUpdate(const dynamic_config::Diff&) {
+            if (cb) cb();
+        }
+
+        std::function<void()> cb;
+    };
+
+    dynamic_config::StorageMock storage{{kDummyConfig, {42, "what"}}, {kIntConfig, 5}};
+    auto source = storage.GetSource();
+    LocalSubscriber subscriber;
+
+    auto scope = source.UpdateAndListen(&subscriber, "test", &LocalSubscriber::OnDiffUpdate);
+
+    LocalSubscriber subscriber2;
+    concurrent::AsyncEventSubscriberScope subscriber2_scope;
+    subscriber.cb = [&] {
+        // Subscribe inside of callback
+        subscriber2_scope = source.UpdateAndListen(&subscriber2, "test2", &LocalSubscriber::OnDiffUpdate);
+    };
+
+    // emit SendEvent() which calls callback which subscribes
+    storage.Extend({});
+
+    subscriber.cb = {};  // cleanup to avoid UAF in dtr (in debug)
+}
+
 USERVER_NAMESPACE_END

core/src/dynamic_config/storage_data.cpp

@@ -32,18 +32,22 @@ StorageData::StorageData() : StorageData(SnapshotData{}) {}
 rcu::ReadablePtr<SnapshotData> StorageData::Read() const { return config_.Read(); }
 
 void StorageData::Update(SnapshotData config, AfterAssignHook after_assign_hook) {
-    const std::lock_guard lock(update_mutex_);
+    std::unique_lock lock(update_mutex_);
 
     std::optional<Snapshot> previous_config;
     {
-        Snapshot current_config(*this);
+        auto current_config = GetSnapshot();
         if (!current_config.GetData().IsEmpty()) previous_config = std::move(current_config);
     }
 
     config_.Assign(std::move(config));
     after_assign_hook();
-
     const Diff diff{std::move(previous_config), GetSnapshot()};
+
+    lock.release();
+    update_mutex_.unlock_and_lock_shared();
+    std::shared_lock tmp_lock{update_mutex_, std::adopt_lock};
+
     diff_channel_.SendEvent(diff);
     snapshot_channel_.SendEvent(GetSnapshot());
 }
@@ -73,7 +77,7 @@ StorageData::DoUpdateAndListen(concurrent::FunctionId id, std::string_view name,
     //
     // As a result, the subscriber receives two `Diff` object with 'new_config' as
     // a `current` field.
-    const std::lock_guard lock(update_mutex_);
+    std::shared_lock lock(update_mutex_);
 
     auto updater = [&, func_copy = func] {
         const Diff diff{std::nullopt, GetSnapshot()};

core/src/dynamic_config/storage_data.hpp

@@ -3,7 +3,7 @@
 #include <userver/concurrent/async_event_channel.hpp>
 #include <userver/dynamic_config/impl/snapshot.hpp>
 #include <userver/dynamic_config/snapshot.hpp>
-#include <userver/engine/mutex.hpp>
+#include <userver/engine/shared_mutex.hpp>
 #include <userver/rcu/rcu.hpp>
 #include <userver/utils/function_ref.hpp>
 
@@ -41,7 +41,7 @@ class StorageData final {
     SnapshotChannel snapshot_channel_;
     DiffChannel diff_channel_;
 
-    engine::Mutex update_mutex_;
+    engine::SharedMutex update_mutex_;
 };
 
 }  // namespace dynamic_config::impl