refactor: add tr_port_forwarding::Mediator (#3855)

* refactor: add a Mediator class to decouple tr_session and tr_port_forwarding

* refactor: add tr_port_forwarding::Mediator::privatePeerPort()

* refactor: add tr_port_forwarding::Mediator::onPortForwarded()

* chore: avoid unnecessary include of timer.h in other headers

* refactor: use a uniform timerMaker() API in mediators

libtransmission/announcer.cc

@@ -32,6 +32,7 @@
 #include "log.h"
 #include "peer-mgr.h" /* tr_peerMgrCompactToPex() */
 #include "session.h"
+#include "timer.h"
 #include "torrent.h"
 #include "tr-assert.h"
 #include "utils.h"

libtransmission/handshake.cc

@@ -6,6 +6,7 @@
 #include <algorithm>
 #include <array>
 #include <cerrno>
+#include <chrono>
 #include <string_view>
 #include <utility>
 
@@ -20,6 +21,7 @@
 #include "handshake.h"
 #include "log.h"
 #include "peer-io.h"
+#include "timer.h"
 #include "tr-assert.h"
 #include "utils.h"
 
@@ -1119,8 +1121,7 @@ tr_handshake* tr_handshakeNew(
     auto* const handshake = new tr_handshake{ std::move(mediator), std::move(io), encryption_mode };
     handshake->done_func = done_func;
     handshake->done_func_user_data = done_func_user_data;
-    handshake->timeout_timer = handshake->mediator->createTimer();
+    handshake->timeout_timer = handshake->mediator->timerMaker().create([handshake]() { tr_handshakeAbort(handshake); });
-    handshake->timeout_timer->setCallback([handshake]() { tr_handshakeAbort(handshake); });
     handshake->timeout_timer->startSingleShot(HandshakeTimeoutSec);
 
     handshake->io->setCallbacks(canRead, nullptr, gotError, handshake);

libtransmission/handshake.h

@@ -17,11 +17,15 @@
 
 #include "net.h" // tr_address
 #include "peer-mse.h" // tr_message_stream_encryption::DH
-#include "timer.h"
 
 /** @addtogroup peers Peers
     @{ */
 
+namespace libtransmission
+{
+class TimerMaker;
+}
+
 class tr_peerIo;
 
 /** @brief opaque struct holding handshake state information.
@@ -55,7 +59,7 @@ public:
 
     [[nodiscard]] virtual std::optional<torrent_info> torrentInfoFromObfuscated(tr_sha1_digest_t const& info_hash) const = 0;
 
-    [[nodiscard]] virtual std::unique_ptr<libtransmission::Timer> createTimer() = 0;
+    [[nodiscard]] virtual libtransmission::TimerMaker& timerMaker() = 0;
 
     [[nodiscard]] virtual bool isDHTEnabled() const = 0;
 

libtransmission/peer-mgr.cc

@@ -41,6 +41,7 @@
 #include "peer-mgr.h"
 #include "peer-msgs.h"
 #include "session.h"
+#include "timer.h"
 #include "torrent.h"
 #include "tr-assert.h"
 #include "tr-dht.h"
@@ -125,9 +126,9 @@ public:
         return tor != nullptr && tr_peerMgrPeerIsSeed(tor, addr);
     }
 
-    [[nodiscard]] std::unique_ptr<libtransmission::Timer> createTimer() override
+    [[nodiscard]] libtransmission::TimerMaker& timerMaker() override
     {
-        return session_.timerMaker().create();
+        return session_.timerMaker();
     }
 
     [[nodiscard]] size_t pad(void* setme, size_t maxlen) const override

libtransmission/peer-msgs.cc

@@ -33,6 +33,7 @@
 #include "peer-msgs.h"
 #include "quark.h"
 #include "session.h"
+#include "timer.h"
 #include "torrent-magnet.h"
 #include "torrent.h"
 #include "tr-assert.h"

libtransmission/port-forwarding.cc

@@ -18,7 +18,6 @@
 #include "port-forwarding-natpmp.h"
 #include "port-forwarding-upnp.h"
 #include "port-forwarding.h"
-#include "session.h"
 #include "timer.h"
 #include "torrent.h"
 #include "tr-assert.h"
@@ -29,9 +28,8 @@ using namespace std::literals;
 class tr_port_forwarding_impl final : public tr_port_forwarding
 {
 public:
-    explicit tr_port_forwarding_impl(tr_session& session)
+    explicit tr_port_forwarding_impl(Mediator& mediator)
-        : session_{ session }
+        : mediator_{ mediator }
-        , timer_maker_{ session.timerMaker() }
     {
     }
 
@@ -105,7 +103,7 @@ private:
 
     void startTimer()
     {
-        timer_ = timer_maker_.create([this]() { this->onTimer(); });
+        timer_ = mediator_.timerMaker().create([this]() { this->onTimer(); });
         restartTimer();
     }
 
@@ -180,7 +178,6 @@ private:
 
     void natPulse(bool do_check)
     {
-        auto& session = session_;
         auto const is_enabled = is_enabled_ && !is_shutting_down_;
 
         if (!natpmp_)
@@ -195,19 +192,23 @@ private:
 
         auto const old_state = state();
 
-        auto const result = natpmp_->pulse(session.private_peer_port, is_enabled);
+        auto const result = natpmp_->pulse(mediator_.privatePeerPort(), is_enabled);
         natpmp_state_ = result.state;
         if (!std::empty(result.public_port) && !std::empty(result.private_port))
         {
-            session.public_peer_port = result.public_port;
+            mediator_.onPortForwarded(result.public_port, result.private_port);
-            session.private_peer_port = result.private_port;
             tr_logAddInfo(fmt::format(
                 _("Mapped private port {private_port} to public port {public_port}"),
-                fmt::arg("public_port", session.public_peer_port.host()),
+                fmt::arg("public_port", result.public_port.host()),
-                fmt::arg("private_port", session.private_peer_port.host())));
+                fmt::arg("private_port", result.private_port.host())));
         }
 
-        upnp_state_ = tr_upnpPulse(upnp_, session.private_peer_port, is_enabled, do_check, session.bind_ipv4.readable());
+        upnp_state_ = tr_upnpPulse(
+            upnp_,
+            mediator_.privatePeerPort(),
+            is_enabled,
+            do_check,
+            mediator_.incomingPeerAddress().readable());
 
         if (auto const new_state = state(); new_state != old_state)
         {
@@ -218,8 +219,7 @@ private:
         }
     }
 
-    tr_session& session_;
+    Mediator& mediator_;
-    libtransmission::TimerMaker& timer_maker_;
 
     bool is_enabled_ = false;
     bool is_shutting_down_ = false;
@@ -234,7 +234,7 @@ private:
     std::unique_ptr<libtransmission::Timer> timer_;
 };
 
-std::unique_ptr<tr_port_forwarding> tr_port_forwarding::create(tr_session& session)
+std::unique_ptr<tr_port_forwarding> tr_port_forwarding::create(Mediator& mediator)
 {
-    return std::make_unique<tr_port_forwarding_impl>(session);
+    return std::make_unique<tr_port_forwarding_impl>(mediator);
 }

libtransmission/port-forwarding.h

@@ -13,20 +13,33 @@
 
 #include "transmission.h" // for tr_port_forwarding_state
 
-struct tr_session;
+#include "net.h"
+
+namespace libtransmission
+{
+class TimerMaker;
+}
 
 class tr_port_forwarding
 {
 public:
+    class Mediator
+    {
+    public:
+        virtual ~Mediator() = default;
+
+        [[nodiscard]] virtual tr_port privatePeerPort() const = 0;
+        [[nodiscard]] virtual tr_address incomingPeerAddress() const = 0;
+        [[nodiscard]] virtual libtransmission::TimerMaker& timerMaker() = 0;
+        virtual void onPortForwarded(tr_port public_port, tr_port private_port) = 0;
+    };
+
+    [[nodiscard]] static std::unique_ptr<tr_port_forwarding> create(Mediator&);
     virtual ~tr_port_forwarding() = default;
 
-    virtual void portChanged() = 0;
-
-    virtual void setEnabled(bool enabled) = 0;
-
     [[nodiscard]] virtual bool isEnabled() const = 0;
-
     [[nodiscard]] virtual tr_port_forwarding_state state() const = 0;
 
-    [[nodiscard]] static std::unique_ptr<tr_port_forwarding> create(tr_session&);
+    virtual void portChanged() = 0;
+    virtual void setEnabled(bool enabled) = 0;
 };

libtransmission/rpc-server.cc

@@ -42,6 +42,7 @@
 #include "rpcimpl.h"
 #include "session-id.h"
 #include "session.h"
+#include "timer.h"
 #include "tr-assert.h"
 #include "tr-strbuf.h"
 #include "trevent.h"

libtransmission/rpc-server.h

@@ -17,13 +17,17 @@
 #include "transmission.h"
 
 #include "net.h"
-#include "timer.h"
 
 struct evhttp;
 struct tr_variant;
 struct tr_rpc_address;
 struct libdeflate_compressor;
 
+namespace libtransmission
+{
+class Timer;
+}
+
 class tr_rpc_server
 {
 public:

libtransmission/session.cc

@@ -731,7 +731,7 @@ void tr_session::initImpl(init_data& data)
 
     this->peerMgr = tr_peerMgrNew(this);
 
-    this->port_forwarding_ = tr_port_forwarding::create(*this);
+    this->port_forwarding_ = tr_port_forwarding::create(port_forwarding_mediator_);
 
     /**
     ***  Blocklist
@@ -753,7 +753,7 @@ void tr_session::initImpl(init_data& data)
 
     if (this->allowsLPD())
     {
-        this->lpd_ = tr_lpd::create(lpd_mediator_, timerMaker(), eventBase());
+        this->lpd_ = tr_lpd::create(lpd_mediator_, eventBase());
     }
 
     tr_utpInit(this);
@@ -2132,7 +2132,7 @@ void tr_sessionSetLPDEnabled(tr_session* session, bool enabled)
             session->is_lpd_enabled_ = enabled;
             if (enabled)
             {
-                session->lpd_ = tr_lpd::create(session->lpd_mediator_, session->timerMaker(), session->eventBase());
+                session->lpd_ = tr_lpd::create(session->lpd_mediator_, session->eventBase());
             }
         });
 }

libtransmission/session.h

@@ -32,14 +32,14 @@
 #include "interned-string.h"
 #include "net.h" // tr_socket_t
 #include "open-files.h"
+#include "port-forwarding.h"
 #include "quark.h"
 #include "session-id.h"
 #include "stats.h"
-#include "timer.h"
 #include "torrents.h"
 #include "tr-lpd.h"
-#include "web.h"
 #include "verify.h"
+#include "web.h"
 
 enum tr_auto_switch_state_t
 {
@@ -62,6 +62,12 @@ struct struct_utp_context;
 struct tr_announcer;
 struct tr_announcer_udp;
 
+namespace libtransmission
+{
+class Timer;
+class TimerMaker;
+} // namespace libtransmission
+
 namespace libtransmission::test
 {
 
@@ -890,6 +896,41 @@ private:
     bool should_scrape_paused_torrents_ = false;
     bool is_incomplete_file_naming_enabled_ = false;
 
+    class PortForwardingMediator final : public tr_port_forwarding::Mediator
+    {
+    public:
+        explicit PortForwardingMediator(tr_session& session)
+            : session_{ session }
+        {
+        }
+
+        [[nodiscard]] tr_address incomingPeerAddress() const override
+        {
+            return session_.bind_ipv4.addr_;
+        }
+
+        [[nodiscard]] tr_port privatePeerPort() const override
+        {
+            return session_.private_peer_port;
+        }
+
+        [[nodiscard]] libtransmission::TimerMaker& timerMaker() override
+        {
+            return session_.timerMaker();
+        }
+
+        void onPortForwarded(tr_port public_port, tr_port private_port) override
+        {
+            session_.public_peer_port = public_port;
+            session_.private_peer_port = private_port;
+        }
+
+    private:
+        tr_session& session_;
+    };
+
+    PortForwardingMediator port_forwarding_mediator_{ *this };
+
     class WebMediator final : public tr_web::Mediator
     {
     public:
@@ -931,6 +972,11 @@ private:
             return session_.allowsLPD();
         }
 
+        [[nodiscard]] libtransmission::TimerMaker& timerMaker() override
+        {
+            return session_.timerMaker();
+        }
+
         [[nodiscard]] std::vector<TorrentInfo> torrents() const override;
 
         bool onPeerFound(std::string_view info_hash_str, tr_address address, tr_port port) override;

libtransmission/tr-dht.cc

@@ -43,6 +43,7 @@
 #include "net.h"
 #include "peer-mgr.h"
 #include "session.h"
+#include "timer.h"
 #include "torrent.h"
 #include "tr-assert.h"
 #include "tr-dht.h"

libtransmission/tr-lpd.cc

@@ -28,6 +28,7 @@
 #include "crypto-utils.h" // for tr_rand_buffer()
 #include "log.h"
 #include "net.h"
+#include "timer.h"
 #include "tr-assert.h"
 #include "tr-lpd.h"
 #include "utils.h" // for tr_net_init()
@@ -203,10 +204,10 @@ std::optional<ParsedAnnounce> parseAnnounceMsg(std::string_view announce)
 class tr_lpd_impl final : public tr_lpd
 {
 public:
-    tr_lpd_impl(Mediator& mediator, libtransmission::TimerMaker& timer_maker, struct event_base* event_base)
+    tr_lpd_impl(Mediator& mediator, struct event_base* event_base)
         : mediator_{ mediator }
-        , announce_timer_{ timer_maker.create([this]() { announceUpkeep(); }) }
+        , announce_timer_{ mediator.timerMaker().create([this]() { announceUpkeep(); }) }
-        , dos_timer_{ timer_maker.create([this]() { dosUpkeep(); }) }
+        , dos_timer_{ mediator.timerMaker().create([this]() { dosUpkeep(); }) }
     {
         if (!init(event_base))
         {
@@ -598,10 +599,7 @@ private:
     static auto constexpr AnnounceScope = int{ TtlSameSubnet }; /**<the maximum scope for LPD datagrams */
 };
 
-std::unique_ptr<tr_lpd> tr_lpd::create(
+std::unique_ptr<tr_lpd> tr_lpd::create(Mediator& mediator, struct event_base* event_base)
-    Mediator& mediator,
-    libtransmission::TimerMaker& timer_maker,
-    struct event_base* event_base)
 {
-    return std::make_unique<tr_lpd_impl>(mediator, timer_maker, event_base);
+    return std::make_unique<tr_lpd_impl>(mediator, event_base);
 }

libtransmission/tr-lpd.h

@@ -17,10 +17,14 @@
 #include "transmission.h"
 
 #include "net.h" // for tr_address, tr_port
-#include "timer.h"
 
 struct event_base;
 
+namespace libtransmission
+{
+class TimerMaker;
+}
+
 class tr_lpd
 {
 public:
@@ -43,6 +47,8 @@ public:
 
         [[nodiscard]] virtual std::vector<TorrentInfo> torrents() const = 0;
 
+        [[nodiscard]] virtual libtransmission::TimerMaker& timerMaker() = 0;
+
         virtual void setNextAnnounceTime(std::string_view info_hash_str, time_t announce_at) = 0;
 
         // returns true if info was used
@@ -50,5 +56,5 @@ public:
     };
 
     virtual ~tr_lpd() = default;
-    static std::unique_ptr<tr_lpd> create(Mediator& mediator, libtransmission::TimerMaker&, event_base* event_base);
+    static std::unique_ptr<tr_lpd> create(Mediator& mediator, event_base* event_base);
 };

libtransmission/tr-utp.cc

@@ -12,13 +12,14 @@
 
 #include "transmission.h"
 
-#include "crypto-utils.h" /* tr_rand_int_weak() */
+#include "crypto-utils.h" // tr_rand_int_weak()
 #include "log.h"
 #include "net.h"
 #include "peer-io.h"
 #include "peer-mgr.h"
 #include "peer-socket.h"
 #include "session.h"
+#include "timer.h"
 #include "tr-utp.h"
 #include "utils.h"
 

libtransmission/watchdir-base.h

@@ -3,6 +3,10 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
+#ifndef LIBTRANSMISSION_WATCHDIR_MODULE
+#error only the wathcdir module should #include this header.
+#endif
+
 #pragma once
 
 #include <algorithm>

libtransmission/watchdir-generic.cc

@@ -3,10 +3,10 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
-#define LIBTRANSMISSION_WATCHDIR_MODULE
-
 #include <utility>
 
+#define LIBTRANSMISSION_WATCHDIR_MODULE
+
 #include "transmission.h"
 
 #include "watchdir-base.h"

libtransmission/watchdir.cc

@@ -3,11 +3,11 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
-#define LIBTRANSMISSION_WATCHDIR_MODULE
-
 #include <chrono>
 #include <set>
 
+#define LIBTRANSMISSION_WATCHDIR_MODULE
+
 #include "transmission.h"
 
 #include "error-types.h"

libtransmission/watchdir.h

@@ -10,8 +10,6 @@
 #include <memory>
 #include <string_view>
 
-#include "timer.h"
-
 extern "C"
 {
     struct event_base;
@@ -20,6 +18,8 @@ extern "C"
 namespace libtransmission
 {
 
+class TimerMaker;
+
 class Watchdir
 {
 public:

libtransmission/webseed.cc

@@ -23,8 +23,9 @@
 #include "cache.h"
 #include "peer-io.h"
 #include "peer-mgr.h"
+#include "timer.h"
 #include "torrent.h"
-#include "trevent.h" /* tr_runInEventThread() */
+#include "trevent.h" // tr_runInEventThread()
 #include "utils.h"
 #include "web-utils.h"
 #include "web.h"
@@ -164,8 +165,8 @@ public:
         , base_url{ url }
         , callback{ callback_in }
         , callback_data{ callback_data_in }
-        , bandwidth_(&tor->bandwidth_)
+        , bandwidth_{ &tor->bandwidth_ }
-        , idle_timer(session->timerMaker().create([this]() { on_idle(this); }))
+        , idle_timer{ session->timerMaker().create([this]() { on_idle(this); }) }
     {
         idle_timer->startRepeating(IdleTimerInterval);
     }

tests/libtransmission/handshake-test.cc

@@ -15,6 +15,7 @@
 #include "handshake.h"
 #include "peer-io.h"
 #include "session.h" // tr_peerIdInit()
+#include "timer.h"
 
 #include "test-fixtures.h"
 
@@ -67,9 +68,9 @@ public:
             return {};
         }
 
-        [[nodiscard]] std::unique_ptr<libtransmission::Timer> createTimer() override
+        [[nodiscard]] libtransmission::TimerMaker& timerMaker() override
         {
-            return session_->timerMaker().create();
+            return session_->timerMaker();
         }
 
         [[nodiscard]] bool isDHTEnabled() const override

tests/libtransmission/lpd-test.cc

@@ -32,7 +32,11 @@ namespace
 class MyMediator final : public tr_lpd::Mediator
 {
 public:
-    MyMediator() = default;
+    explicit MyMediator(tr_session& session)
+        : session_{ session }
+    {
+    }
+
     ~MyMediator() override = default;
 
     [[nodiscard]] tr_port port() const override
@@ -50,6 +54,11 @@ public:
         return torrents_;
     }
 
+    [[nodiscard]] libtransmission::TimerMaker& timerMaker() override
+    {
+        return session_.timerMaker();
+    }
+
     void setNextAnnounceTime(std::string_view info_hash_str, time_t announce_after) override
     {
         for (auto& tor : torrents_)
@@ -68,6 +77,7 @@ public:
         return found_returns_;
     }
 
+    tr_session& session_;
     tr_port port_ = tr_port::fromHost(51413);
     bool allows_lpd_ = true;
     std::vector<TorrentInfo> torrents_;
@@ -91,16 +101,16 @@ auto makeRandomHashString()
 
 TEST_F(LpdTest, HelloWorld)
 {
-    auto mediator = MyMediator{};
+    auto mediator = MyMediator{ *session_ };
-    auto lpd = tr_lpd::create(mediator, session_->timerMaker(), session_->eventBase());
+    auto lpd = tr_lpd::create(mediator, session_->eventBase());
     EXPECT_TRUE(lpd);
     EXPECT_EQ(0U, std::size(mediator.found_));
 }
 
 TEST_F(LpdTest, CanAnnounceAndRead)
 {
-    auto mediator_a = MyMediator{};
+    auto mediator_a = MyMediator{ *session_ };
-    auto lpd_a = tr_lpd::create(mediator_a, session_->timerMaker(), session_->eventBase());
+    auto lpd_a = tr_lpd::create(mediator_a, session_->eventBase());
     EXPECT_TRUE(lpd_a);
 
     auto const info_hash_str = makeRandomHashString();
@@ -110,9 +120,9 @@ TEST_F(LpdTest, CanAnnounceAndRead)
     info.allows_lpd = true;
     info.announce_after = 0; // never announced
 
-    auto mediator_b = MyMediator{};
+    auto mediator_b = MyMediator{ *session_ };
     mediator_b.torrents_.push_back(info);
-    auto lpd_b = tr_lpd::create(mediator_b, session_->timerMaker(), session_->eventBase());
+    auto lpd_b = tr_lpd::create(mediator_b, session_->eventBase());
 
     waitFor([&mediator_a]() { return !std::empty(mediator_a.found_); }, 1s);
     EXPECT_EQ(1U, mediator_a.found_.count(info_hash_str));
@@ -121,13 +131,13 @@ TEST_F(LpdTest, CanAnnounceAndRead)
 
 TEST_F(LpdTest, canMultiAnnounce)
 {
-    auto mediator_a = MyMediator{};
+    auto mediator_a = MyMediator{ *session_ };
-    auto lpd_a = tr_lpd::create(mediator_a, session_->timerMaker(), session_->eventBase());
+    auto lpd_a = tr_lpd::create(mediator_a, session_->eventBase());
     EXPECT_TRUE(lpd_a);
 
     auto info_hash_strings = std::array<std::string, 2>{};
     auto infos = std::array<tr_lpd::Mediator::TorrentInfo, 2>{};
-    auto mediator_b = MyMediator{};
+    auto mediator_b = MyMediator{ *session_ };
     for (size_t i = 0; i < std::size(info_hash_strings); ++i)
     {
         auto& info_hash_string = info_hash_strings[i];
@@ -146,7 +156,7 @@ TEST_F(LpdTest, canMultiAnnounce)
         mediator_b.torrents_.push_back(info);
     }
 
-    auto lpd_b = tr_lpd::create(mediator_b, session_->timerMaker(), session_->eventBase());
+    auto lpd_b = tr_lpd::create(mediator_b, session_->eventBase());
     waitFor([&mediator_a]() { return !std::empty(mediator_a.found_); }, 1s);
 
     for (auto const& info : infos)
@@ -157,14 +167,14 @@ TEST_F(LpdTest, canMultiAnnounce)
 
 TEST_F(LpdTest, DoesNotReannounceTooSoon)
 {
-    auto mediator_a = MyMediator{};
+    auto mediator_a = MyMediator{ *session_ };
-    auto lpd_a = tr_lpd::create(mediator_a, session_->timerMaker(), session_->eventBase());
+    auto lpd_a = tr_lpd::create(mediator_a, session_->eventBase());
     EXPECT_TRUE(lpd_a);
 
     // similar to canMultiAnnounce...
     auto info_hash_strings = std::array<std::string, 2>{};
     auto infos = std::array<tr_lpd::Mediator::TorrentInfo, 2>{};
-    auto mediator_b = MyMediator{};
+    auto mediator_b = MyMediator{ *session_ };
     for (size_t i = 0; i < std::size(info_hash_strings); ++i)
     {
         auto& info_hash_string = info_hash_strings[i];
@@ -188,7 +198,7 @@ TEST_F(LpdTest, DoesNotReannounceTooSoon)
         mediator_b.torrents_.push_back(info);
     }
 
-    auto lpd_b = tr_lpd::create(mediator_b, session_->timerMaker(), session_->eventBase());
+    auto lpd_b = tr_lpd::create(mediator_b, session_->eventBase());
     waitFor([&mediator_a]() { return !std::empty(mediator_a.found_); }, 1s);
 
     for (auto& info : infos)