From d5402b8527a87a887b516d5cdf630acb54ecbcb5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 29 Sep 2025 16:35:59 -0400 Subject: [PATCH 1/3] Encapsulate `curlFileTransfer::State:quit` It is allowed to read it, and to set it to `false`, but not to set it to `true`. --- src/libstore/filetransfer.cc | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index a162df1ad..72153dfdd 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -594,10 +594,21 @@ struct curlFileTransfer : public FileTransfer } }; - bool quit = false; std:: priority_queue, std::vector>, EmbargoComparator> incoming; + private: + bool quitting = false; + public: + void quit() + { + quitting = true; + } + + bool isQuitting() + { + return quitting; + } }; Sync state_; @@ -649,7 +660,7 @@ struct curlFileTransfer : public FileTransfer /* Signal the worker thread to exit. */ { auto state(state_.lock()); - state->quit = true; + state->quit(); } #ifndef _WIN32 // TODO need graceful async exit support on Windows? writeFull(wakeupPipe.writeSide.get(), " ", false); @@ -750,7 +761,7 @@ struct curlFileTransfer : public FileTransfer break; } } - quit = state->quit; + quit = state->isQuitting(); } for (auto & item : incoming) { @@ -778,7 +789,7 @@ struct curlFileTransfer : public FileTransfer auto state(state_.lock()); while (!state->incoming.empty()) state->incoming.pop(); - state->quit = true; + state->quit(); } } @@ -789,7 +800,7 @@ struct curlFileTransfer : public FileTransfer { auto state(state_.lock()); - if (state->quit) + if (state->isQuitting()) throw nix::Error("cannot enqueue download request because the download thread is shutting down"); state->incoming.push(item); } @@ -845,7 +856,7 @@ ref getFileTransfer() { static ref fileTransfer = makeCurlFileTransfer(); - if (fileTransfer->state_.lock()->quit) + if (fileTransfer->state_.lock()->isQuitting()) fileTransfer = makeCurlFileTransfer(); return fileTransfer; From 1f65b08d947d9ab7eb397eebe49609963e003641 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 29 Sep 2025 16:37:12 -0400 Subject: [PATCH 2/3] `curlFileTransfer::State:quit` emptys the queue Whoever first calls `quit` now empties the queue, instead of waiting for the worker thread to do it. (Note that in the unwinding case, the worker thread is still the first to call `quit`, though.) --- src/libstore/filetransfer.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 72153dfdd..f8f5b48e0 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -603,6 +603,9 @@ struct curlFileTransfer : public FileTransfer void quit() { quitting = true; + /* We wil not be processing any more incomming requests */ + while (!incoming.empty()) + incoming.pop(); } bool isQuitting() @@ -787,8 +790,6 @@ struct curlFileTransfer : public FileTransfer { auto state(state_.lock()); - while (!state->incoming.empty()) - state->incoming.pop(); state->quit(); } } From 86fb5b24a9cb528d87cb02efb89483353a4b6c44 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 29 Sep 2025 16:43:45 -0400 Subject: [PATCH 3/3] `curlFileTransfer::workerThreadEntry` Only call `quit` if we need to. --- src/libstore/filetransfer.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index f8f5b48e0..59fc75ed0 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -781,14 +781,18 @@ struct curlFileTransfer : public FileTransfer void workerThreadEntry() { + // Unwinding or because someone called `quit`. + bool normalExit = true; try { workerThreadMain(); } catch (nix::Interrupted & e) { + normalExit = false; } catch (std::exception & e) { printError("unexpected error in download thread: %s", e.what()); + normalExit = false; } - { + if (!normalExit) { auto state(state_.lock()); state->quit(); }