refactor: fix thread sanitizer warnings in tr_sessionClose() (#4130)

libtransmission/session-thread.cc

@@ -3,6 +3,7 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
+#include <atomic>
 #include <chrono>
 #include <condition_variable>
 #include <functional>
@@ -160,7 +161,7 @@ public:
         thread_id_ = thread_.get_id();
 
         // wait for the session thread's main loop to start
-        is_looping_cv_.wait(lock, [this]() { return is_looping_; });
+        is_looping_cv_.wait(lock, [this]() { return is_looping_.load(); });
     }
 
     tr_session_thread_impl(tr_session_thread_impl&&) = delete;
@@ -285,9 +286,9 @@ private:
 
     std::mutex is_looping_mutex_;
     std::condition_variable is_looping_cv_;
-    bool is_looping_ = false;
+    std::atomic<bool> is_looping_ = false;
 
-    bool is_shutting_down_ = false;
+    std::atomic<bool> is_shutting_down_ = false;
     static constexpr std::chrono::seconds Deadline = 5s;
 };
 

libtransmission/session.cc

@@ -63,8 +63,6 @@
 
 using namespace std::literals;
 
-std::recursive_mutex tr_session::session_mutex;
-
 static auto constexpr SaveIntervalSecs = 360s;
 
 static void bandwidthGroupRead(tr_session* session, std::string_view config_dir);
@@ -1162,7 +1160,13 @@ double tr_sessionGetRawSpeed_KBps(tr_session const* session, tr_direction dir)
     return tr_toSpeedKBps(tr_sessionGetRawSpeed_Bps(session, dir));
 }
 
-void tr_session::closeImplPart1()
+struct tr_session::is_closed_data
+{
+    std::mutex is_closed_mutex;
+    std::condition_variable is_closed_cv;
+};
+
+void tr_session::closeImplPart1(is_closed_data* closed_data)
 {
     is_closing_ = true;
 
@@ -1211,11 +1215,11 @@ void tr_session::closeImplPart1()
 
     // recycle the now-unused save_timer_ here to wait for UDP shutdown
     TR_ASSERT(!save_timer_);
-    save_timer_ = timerMaker().create([this]() { closeImplPart2(); });
+    save_timer_ = timerMaker().create([this, closed_data]() { closeImplPart2(closed_data); });
     save_timer_->startRepeating(50ms);
 }
 
-void tr_session::closeImplPart2()
+void tr_session::closeImplPart2(is_closed_data* closed_data)
 {
     // try to keep the UDP announcer alive long enough to send out
     // all the &event=stopped tracker announces
@@ -1234,45 +1238,25 @@ void tr_session::closeImplPart2()
     peer_mgr_.reset();
     tr_utpClose(this);
     openFiles().closeAll();
+
+    // tada we are done!
+    closed_data->is_closed_mutex.lock();
     is_closed_ = true;
+    closed_data->is_closed_mutex.unlock();
+    closed_data->is_closed_cv.notify_one();
 }
 
 void tr_sessionClose(tr_session* session)
 {
     TR_ASSERT(session != nullptr);
-
-    static auto constexpr DeadlineSecs = 10s;
-    auto const deadline = std::chrono::steady_clock::now() + DeadlineSecs;
-    auto const deadline_reached = [deadline]()
-    {
-        return std::chrono::steady_clock::now() >= deadline;
-    };
+    TR_ASSERT(!session->amInSessionThread());
 
     tr_logAddInfo(fmt::format(_("Transmission version {version} shutting down"), fmt::arg("version", LONG_VERSION_STRING)));
 
-    /* close the session */
-    session->runInSessionThread([session]() { session->closeImplPart1(); });
-
-    while (!session->isClosed() && !deadline_reached())
-    {
-        tr_logAddTrace("waiting for the libtransmission thread to finish");
-        tr_wait_msec(10);
-    }
-
-    // There's usually a bit of housekeeping to do during shutdown,
-    // e.g. sending out `event=stopped` announcements to trackers,
-    // so wait a bit for the session thread to close.
-    while (!deadline_reached() && (!session->web_->isClosed() || session->announcer != nullptr || session->announcer_udp_))
-    {
-        tr_logAddTrace(fmt::format(
-            "waiting on port unmap ({}) or announcer ({})... now {}",
-            fmt::ptr(session->port_forwarding_.get()),
-            fmt::ptr(session->announcer),
-            time(nullptr)));
-        tr_wait_msec(50);
-    }
-
-    session->web_.reset();
+    auto closed_data = tr_session::is_closed_data{};
+    auto lock = std::unique_lock{ closed_data.is_closed_mutex };
+    session->runInSessionThread([session, &closed_data]() { session->closeImplPart1(&closed_data); });
+    closed_data.is_closed_cv.wait_for(lock, 12s, [session]() { return session->is_closed_.load(); });
 
     delete session;
 }

libtransmission/session.h

@@ -12,6 +12,7 @@
 #define TR_NAME "Transmission"
 
 #include <array>
+#include <atomic>
 #include <cstddef> // size_t
 #include <cstdint> // uintX_t
 #include <memory>
@@ -679,7 +680,7 @@ public:
         return is_closing_;
     }
 
-    [[nodiscard]] constexpr auto isClosed() const noexcept
+    [[nodiscard]] bool isClosed() const noexcept
     {
         return is_closed_;
     }
@@ -897,8 +898,9 @@ private:
     void setSettings(tr_variant* settings_dict, bool force);
     void setSettings(tr_session_settings settings, bool force);
 
-    void closeImplPart1();
-    void closeImplPart2();
+    struct is_closed_data;
+    void closeImplPart1(is_closed_data*);
+    void closeImplPart2(is_closed_data*);
 
     void onNowTimer();
 
@@ -1001,7 +1003,7 @@ private:
 
     /// static fields
 
-    static std::recursive_mutex session_mutex;
+    static inline std::recursive_mutex session_mutex;
 
     /// trivial type fields
 
@@ -1044,7 +1046,7 @@ private:
     uint16_t peer_count_ = 0;
 
     bool is_closing_ = false;
-    bool is_closed_ = false;
+    std::atomic<bool> is_closed_ = false;
 
     /// fields that aren't trivial,
     /// but are self-contained / have no interdependencies

libtransmission/web.cc

@@ -4,6 +4,7 @@
 // License text can be found in the licenses/ folder.
 
 #include <algorithm>
+#include <atomic>
 #include <array>
 #include <condition_variable>
 #include <list>
@@ -307,7 +308,7 @@ public:
         CloseNow // exit now even if tasks are running
     };
 
-    RunMode run_mode = RunMode::Run;
+    std::atomic<RunMode> run_mode = RunMode::Run;
 
     static size_t onDataReceived(void* data, size_t size, size_t nmemb, void* vtask)
     {
@@ -595,7 +596,7 @@ public:
 
     static std::once_flag curl_init_flag;
 
-    bool is_closed_ = false;
+    std::atomic<bool> is_closed_ = false;
 
     std::multimap<uint64_t /*tr_time_msec()*/, CURL*> paused_easy_handles;