Retry UDP tracker requests and cleanup

Retry accorduing to the doc, currently synchronous.

Make the request setup code cleaner.

src/peer.cpp

@@ -71,7 +71,7 @@ PeerConnection::PeerConnection(Peer& peer,
 }
 
 void PeerConnection::listen() {
-  logger()->info("{} port={}", PRETTY_FUNCTION, m_listening_port.get());
+  logger()->info("PeerConnection listening on port={}", m_listening_port.get());
   // if (!acceptor_.is_open()) {
   const asio::socket_base::reuse_address option(true);
   acceptor_.set_option(option);
@@ -111,7 +111,7 @@ void PeerConnection::write(const std::string& msg) {
 }
 
 void PeerConnection::write(const optional<Url>& url, const std::string& msg) {
-  logger()->debug(PRETTY_FUNCTION);
+  logger()->trace(PRETTY_FUNCTION);
 
   if (!m_msg.empty()) {
     throw runtime_error("Message not empty");
@@ -135,7 +135,7 @@ void PeerConnection::write(const optional<Url>& url, const std::string& msg) {
 
 void PeerConnection::handle_resolve(const asio::error_code& err,
                                     tcp::resolver::iterator endpoint_iterator) {
-  logger()->debug(PRETTY_FUNCTION);
+  logger()->trace(PRETTY_FUNCTION);
   if (!err) {
     // Attempt a connection to the first endpoint in the list. Each endpoint
     // will be tried until we successfully establish a connection.
@@ -148,7 +148,7 @@ void PeerConnection::handle_resolve(const asio::error_code& err,
     socket_.async_connect(endpoint, [this, it = ++endpoint_iterator](
                                         auto&& ec) { handle_connect(ec, it); });
   } else {
-    logger()->error("Resolve failed: {}", err.message());
+    logger()->error("Resolve failed for {}: {}", peer().str(), err.message());
   }
 }
 
@@ -187,7 +187,7 @@ void PeerConnection::send(bool start_read) {
 
 void PeerConnection::handle_connect(const asio::error_code& err,
                                     tcp::resolver::iterator endpoint_iterator) {
-  logger()->debug(PRETTY_FUNCTION);
+  logger()->trace(PRETTY_FUNCTION);
   if (!err) {
     // The connection was successful. Send the request.
     if (!m_connected) {

src/retry.hpp

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <chrono>
-#include <format>
 #include <thread>
 
 #include "logger.hpp"
@@ -59,4 +58,4 @@ inline auto retry_call(
   throw std::runtime_error("retry_call should not end up here");
 }
 
-}  // namespace zit
+}  // namespace zit

src/torrent.cpp

@@ -8,6 +8,7 @@
 #include "peer.hpp"
 #include "piece.hpp"
 #include "random.hpp"
+#include "retry.hpp"
 #include "sha1.hpp"
 #include "string_utils.hpp"
 #include "timer.hpp"
@@ -502,11 +503,25 @@ Torrent::http_tracker_request(const Url& announce_url, TrackerEvent event) {
 
 /**
  * Protocol documented here: https://libtorrent.org/udp_tracker_protocol.html
+ *
+ * The requests are currently retired and synchronous so can be slow
+ * if all requests fail. A possible improvement would be to look up
+ * multiple trackers at the same time with async connections.
  */
 class UDPTrackerRequest {
   using ClockType = std::chrono::high_resolution_clock;
   using TimePoint = std::chrono::time_point<ClockType>;
 
+  template <std::integral T, typename Container>
+  void append_big_endian(T val, Container& container) {
+    ranges::move(to_big_endian<T>(val), std::back_inserter(container));
+  }
+
+  template <typename Container>
+  void append_big_endian(bytes&& data, Container& container) {
+    ranges::move(std::move(data), std::back_inserter(container));
+  }
+
  public:
   UDPTrackerRequest(Url announce_url, Torrent& torrent)
       : m_announce_url(std::move(announce_url)), m_torrent(torrent) {}
@@ -516,6 +531,12 @@ class UDPTrackerRequest {
    */
   std::pair<bool, std::vector<std::shared_ptr<Peer>>> announce(
       Torrent::TrackerEvent event) {
+    if (logger()->should_log(spdlog::level::debug)) {
+      logger()->debug("Tracker request:\n{}", m_announce_url);
+    } else {
+      logger()->info("Tracker request: {}", m_announce_url.str());
+    }
+
     connect();
 
     if (!m_connection_id) {
@@ -526,52 +547,47 @@ class UDPTrackerRequest {
     const auto transaction_id = random_value<int32_t>();
 
     bytes announce_request;
-    ranges::move(to_big_endian<int64_t>(m_connection_id.value()),
-                 back_inserter(announce_request));
-    ranges::move(
-        to_big_endian<int32_t>(static_cast<int32_t>(UdpAction::ANNOUNCE)),
-        back_inserter(announce_request));
-    ranges::move(to_big_endian<int32_t>(transaction_id),
-                 back_inserter(announce_request));
-    ranges::move(m_torrent.info_hash().bytes(),
-                 back_inserter(announce_request));
-    ranges::move(to_bytes(m_torrent.peer_id()),
-                 back_inserter(announce_request));
-    ranges::move(to_big_endian<int64_t>(m_torrent.downloaded()),
-                 back_inserter(announce_request));
-    ranges::move(to_big_endian<int64_t>(m_torrent.left()),
-                 back_inserter(announce_request));
+    append_big_endian(m_connection_id.value(), announce_request);
+    append_big_endian(static_cast<int32_t>(UdpAction::ANNOUNCE),
+                      announce_request);
+    append_big_endian(transaction_id, announce_request);
+    append_big_endian(m_torrent.info_hash().bytes(), announce_request);
+    append_big_endian(to_bytes(m_torrent.peer_id()), announce_request);
+    append_big_endian(m_torrent.downloaded(), announce_request);
+    append_big_endian(m_torrent.left(), announce_request);
     // FIXME: This is the upload bytes count - not yet keeping track of that
     constexpr int64_t uploaded_bytes{0};
-    ranges::move(to_big_endian<int64_t>(uploaded_bytes),
-                 back_inserter(announce_request));
-    ranges::move(to_big_endian<int32_t>(static_cast<int32_t>(event)),
-                 back_inserter(announce_request));
+    append_big_endian(uploaded_bytes, announce_request);
+    append_big_endian(static_cast<int32_t>(event), announce_request);
     // Using 0 to indicate the sender IP (but could be an explicit IP)
     constexpr uint32_t my_ip{0};
-    ranges::move(to_big_endian<uint32_t>(my_ip),
-                 back_inserter(announce_request));
+    append_big_endian(my_ip, announce_request);
     const auto key = random_value<uint32_t>();
-    ranges::move(to_big_endian<uint32_t>(key), back_inserter(announce_request));
+    append_big_endian(key, announce_request);
     constexpr int32_t max_peers_wanted{50};
-    ranges::move(to_big_endian<int32_t>(max_peers_wanted),
-                 back_inserter(announce_request));
-    ranges::move(to_big_endian<uint16_t>(m_torrent.listening_port().get()),
-                 back_inserter(announce_request));
+    append_big_endian(max_peers_wanted, announce_request);
+    append_big_endian(m_torrent.listening_port().get(), announce_request);
     // FIXME: This is related to authentication. Not supported at the moment.
     constexpr uint16_t extensions{0};
-    ranges::move(to_big_endian<uint16_t>(extensions),
-                 back_inserter(announce_request));
+    append_big_endian(extensions, announce_request);
 
     assert(announce_request.size() == 100);
 
-    // TODO: According to doc we can retry this for 60s. Add retry support.
-    const auto announce_response =
-        Net::udpRequest(m_announce_url, announce_request, 15s);
-    if (announce_response.empty()) {
+    const auto maybe_announce_response = retry_call(
+        [&]() -> std::optional<bytes> {
+          auto response =
+              Net::udpRequest(m_announce_url, announce_request, 15s);
+          if (response.empty()) {
+            return {};
+          }
+          return response;
+        },
+        4, 15s);
+    if (!maybe_announce_response) {
       logger()->debug("UDP Tracker request: empty announce response");
       return {true, {}};
     }
+    const auto& announce_response = maybe_announce_response.value();
 
     const auto announce_reply_action =
         toUdpAction(from_big_endian<int32_t>(announce_response));
@@ -668,22 +684,28 @@ class UDPTrackerRequest {
     auto transaction_id = random_value<int32_t>();
 
     bytes connect_request;
-    ranges::move(to_big_endian<int64_t>(connection_id),
-                 back_inserter(connect_request));
-    ranges::move(
-        to_big_endian<int32_t>(static_cast<int32_t>(UdpAction::CONNECT)),
-        back_inserter(connect_request));
-    ranges::move(to_big_endian<int32_t>(transaction_id),
-                 back_inserter(connect_request));
-
-    // TODO: According to doc we can retry this for 60s. Add retry support.
-    const auto connect_response =
-        Net::udpRequest(m_announce_url, connect_request, 15s);
-    if (connect_response.empty()) {
+    append_big_endian(connection_id, connect_request);
+    append_big_endian(static_cast<int32_t>(UdpAction::CONNECT),
+                      connect_request);
+    append_big_endian(transaction_id, connect_request);
+
+    const auto maybe_connect_response = retry_call(
+        [&]() -> std::optional<bytes> {
+          auto response = Net::udpRequest(m_announce_url, connect_request, 15s);
+          if (response.empty()) {
+            return {};
+          }
+          return response;
+        },
+        4, 15s);
+
+    if (!maybe_connect_response) {
       logger()->debug("UDP Tracker request: empty connect response");
       return false;
     }
 
+    const auto& connect_response = maybe_connect_response.value();
+
     if (connect_response.size() < 16) {
       logger()->debug("UDP Tracker request: too short connect response");
       return false;

src/types.hpp

@@ -103,7 +103,7 @@ static constexpr std::byte operator""_b(char arg) noexcept {
 /**
  * Extract big endian value to int
  */
-template <typename T>
+template <std::integral T>
 static inline T from_big_endian(const bytes& buf, bytes::size_type offset = 0) {
   constexpr auto size = sizeof(T);
   static_assert(size == 2 || size == 4 || size == 8,
@@ -140,7 +140,7 @@ static inline T from_big_endian(const bytes& buf, bytes::size_type offset = 0) {
 /**
  * Convert host int to byte vector in network byte order.
  */
-template <typename T>
+template <std::integral T>
 static inline bytes to_big_endian(T val) {
   constexpr auto size = sizeof(T);
   static_assert(size == 2 || size == 4 || size == 8,

tests/CMakeLists.txt

@@ -35,6 +35,7 @@ add_executable(zitest
   test_net.cpp
   test_peer.cpp
   test_random.cpp
+  test_retry.cpp
   test_sha1.cpp
   test_string_utils.cpp
   test_types.cpp