From af27d1ecd8505ce7cabd5bf42eecf7d4e194fda7 Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sat, 26 Oct 2024 17:33:14 +0200
Subject: [PATCH] libstore: make baseRetryTimeMs a FileTransfer property

we don't even need this outside of tests. maybe we should not do
automatic retries at this level at all and use retrying wrappers
instead? at some point we may have to do this, but not just yet.

Change-Id: If0088aa55215be81f1770c25b3bb1b5268c65cf8
---
 src/libstore/filetransfer.cc        | 18 ++++++++++--------
 src/libstore/filetransfer.hh        |  3 +--
 tests/unit/libstore/filetransfer.cc |  6 ++----
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc
index 82961e547..f62b414e3 100644
--- a/src/libstore/filetransfer.cc
+++ b/src/libstore/filetransfer.cc
@@ -39,6 +39,7 @@ struct curlFileTransfer : public FileTransfer
 
     std::random_device rd;
     std::mt19937 mt19937;
+    const unsigned int baseRetryTimeMs;
 
     struct TransferItem : public std::enable_shared_from_this<TransferItem>
     {
@@ -442,7 +443,7 @@ struct curlFileTransfer : public FileTransfer
                         || writtenToSink == 0
                         || (acceptRanges && encoding.empty())))
                 {
-                    int ms = request.baseRetryTimeMs * std::pow(2.0f, attempt - 1 + std::uniform_real_distribution<>(0.0, 0.5)(fileTransfer.mt19937));
+                    int ms = fileTransfer.baseRetryTimeMs * std::pow(2.0f, attempt - 1 + std::uniform_real_distribution<>(0.0, 0.5)(fileTransfer.mt19937));
                     if (writtenToSink)
                         warn("%s; retrying from offset %d in %d ms", exc.what(), writtenToSink, ms);
                     else
@@ -471,8 +472,9 @@ struct curlFileTransfer : public FileTransfer
 
     std::thread workerThread;
 
-    curlFileTransfer()
+    curlFileTransfer(unsigned int baseRetryTimeMs)
         : mt19937(rd())
+        , baseRetryTimeMs(baseRetryTimeMs)
     {
         static std::once_flag globalInit;
         std::call_once(globalInit, curl_global_init, CURL_GLOBAL_ALL);
@@ -874,24 +876,24 @@ struct curlFileTransfer : public FileTransfer
     }
 };
 
-ref<curlFileTransfer> makeCurlFileTransfer()
+ref<curlFileTransfer> makeCurlFileTransfer(std::optional<unsigned int> baseRetryTimeMs)
 {
-    return make_ref<curlFileTransfer>();
+    return make_ref<curlFileTransfer>(baseRetryTimeMs.value_or(250));
 }
 
 ref<FileTransfer> getFileTransfer()
 {
-    static ref<curlFileTransfer> fileTransfer = makeCurlFileTransfer();
+    static ref<curlFileTransfer> fileTransfer = makeCurlFileTransfer({});
 
     if (fileTransfer->state_.lock()->quit)
-        fileTransfer = makeCurlFileTransfer();
+        fileTransfer = makeCurlFileTransfer({});
 
     return fileTransfer;
 }
 
-ref<FileTransfer> makeFileTransfer()
+ref<FileTransfer> makeFileTransfer(std::optional<unsigned int> baseRetryTimeMs)
 {
-    return makeCurlFileTransfer();
+    return makeCurlFileTransfer(baseRetryTimeMs);
 }
 
 template<typename... Args>
diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh
index 38a9243ad..a53ef7af9 100644
--- a/src/libstore/filetransfer.hh
+++ b/src/libstore/filetransfer.hh
@@ -60,7 +60,6 @@ struct FileTransferRequest
     bool verifyTLS = true;
     bool head = false;
     size_t tries = fileTransferSettings.tries;
-    unsigned int baseRetryTimeMs = 250;
 
     FileTransferRequest(std::string_view uri)
         : uri(uri) { }
@@ -123,7 +122,7 @@ ref<FileTransfer> getFileTransfer();
  *
  * Prefer getFileTransfer() to this; see its docs for why.
  */
-ref<FileTransfer> makeFileTransfer();
+ref<FileTransfer> makeFileTransfer(std::optional<unsigned int> baseRetryTimeMs = {});
 
 class FileTransferError : public Error
 {
diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc
index 96901aae2..3c152ee84 100644
--- a/tests/unit/libstore/filetransfer.cc
+++ b/tests/unit/libstore/filetransfer.cc
@@ -178,9 +178,8 @@ TEST(FileTransfer, NOT_ON_DARWIN(defersFailures))
         // might only do so once its internal buffer has already been filled.)
         return std::string(1024 * 1024, ' ');
     });
-    auto ft = makeFileTransfer();
+    auto ft = makeFileTransfer(0);
     FileTransferRequest req(fmt("http://[::1]:%d/index", port));
-    req.baseRetryTimeMs = 0;
     auto src = ft->download(std::move(req));
     ASSERT_THROW(src->drain(), FileTransferError);
 }
@@ -216,9 +215,8 @@ TEST(FileTransfer, usesIntermediateLinkHeaders)
          [] { return ""; }},
         {"200 ok", "content-length: 1\r\n", [] { return "a"; }},
     });
-    auto ft = makeFileTransfer();
+    auto ft = makeFileTransfer(0);
     FileTransferRequest req(fmt("http://[::1]:%d/first", port));
-    req.baseRetryTimeMs = 0;
     auto result = ft->enqueueDownload(req).get();
     ASSERT_EQ(result.immutableUrl, "http://foo");
 }