From 220251ba51e7d8e47553d3edae0e736b2327011d Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 27 Oct 2024 18:27:11 +0100 Subject: [PATCH] libstore: remove FiletransferRequest::head add a method to FileTransfer that provides this functionality instead. Change-Id: Ic1933a5df76a109c248c9c5efea065356b20a6f9 --- src/libstore/filetransfer.cc | 42 +++++++++++++++++++------ src/libstore/filetransfer.hh | 13 +++++++- src/libstore/http-binary-cache-store.cc | 9 +----- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 3a4ff04c3..21fe5715e 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -48,6 +48,7 @@ struct curlFileTransfer : public FileTransfer FileTransferResult result; Activity act; std::optional uploadData; + bool noBody = false; // \equiv HTTP HEAD, don't download data bool done = false; // whether either the success or failure function has been called std::packaged_task callback; std::function dataCallback; @@ -92,7 +93,8 @@ struct curlFileTransfer : public FileTransfer ActivityId parentAct, std::invocable auto callback, std::function dataCallback, - std::optional uploadData + std::optional uploadData, + bool noBody ) : fileTransfer(fileTransfer) , request(request) @@ -100,6 +102,7 @@ struct curlFileTransfer : public FileTransfer fmt(uploadData ? "uploading '%s'" : "downloading '%s'", request.uri), {request.uri}, parentAct) , uploadData(std::move(uploadData)) + , noBody(noBody) , callback([cb{std::move(callback)}] (std::exception_ptr ex, FileTransferResult r) { cb(ex); return r; @@ -299,7 +302,7 @@ struct curlFileTransfer : public FileTransfer if (settings.downloadSpeed.get() > 0) curl_easy_setopt(req, CURLOPT_MAX_RECV_SPEED_LARGE, (curl_off_t) (settings.downloadSpeed.get() * 1024)); - if (request.head) + if (noBody) curl_easy_setopt(req, CURLOPT_NOBODY, 1); if (uploadData) { @@ -661,17 +664,18 @@ struct curlFileTransfer : public FileTransfer std::future enqueueDownload(const FileTransferRequest & request) override { - return enqueueFileTransfer(request, std::nullopt); + return enqueueFileTransfer(request, std::nullopt, false); } std::future enqueueUpload(const FileTransferRequest & request, std::string data) override { - return enqueueFileTransfer(request, std::move(data)); + return enqueueFileTransfer(request, std::move(data), false); } - std::future - enqueueFileTransfer(const FileTransferRequest & request, std::optional data) + std::future enqueueFileTransfer( + const FileTransferRequest & request, std::optional data, bool noBody + ) { return enqueueFileTransfer( request, @@ -681,14 +685,16 @@ struct curlFileTransfer : public FileTransfer } }, {}, - std::move(data) + std::move(data), + noBody ); } std::future enqueueFileTransfer(const FileTransferRequest & request, std::invocable auto callback, std::function dataCallback, - std::optional data + std::optional data, + bool noBody ) { /* Ugly hack to support s3:// URIs. */ @@ -724,11 +730,26 @@ struct curlFileTransfer : public FileTransfer getCurActivity(), std::move(callback), std::move(dataCallback), - std::move(data) + std::move(data), + noBody )) ->callback.get_future(); } + bool exists(std::string_view uri) override + { + try { + enqueueFileTransfer(FileTransferRequest{uri}, std::nullopt, true).get(); + return true; + } catch (FileTransferError & e) { + /* S3 buckets return 403 if a file doesn't exist and the + bucket is unlistable, so treat 403 as 404. */ + if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden) + return false; + throw; + } + } + box_ptr download(FileTransferRequest && request) override { struct State { @@ -776,7 +797,8 @@ struct curlFileTransfer : public FileTransfer state->data.append(data); state->avail.notify_one(); }, - std::nullopt + std::nullopt, + false ); struct InnerSource : Source diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 63e30aa0c..e246eab80 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -58,7 +58,6 @@ struct FileTransferRequest Headers headers; std::string expectedETag; bool verifyTLS = true; - bool head = false; FileTransferRequest(std::string_view uri) : uri(uri) { } @@ -96,6 +95,18 @@ struct FileTransfer virtual std::future enqueueUpload(const FileTransferRequest & request, std::string data) = 0; + /** + * Checks whether the given URI exists. For historical reasons this function + * treats HTTP 403 responses like HTTP 404 responses and returns `false` for + * both. This was originally done to handle unlistable S3 buckets, which may + * return 403 (not 404) if the reuqested object doesn't exist in the bucket. + * + * ## Bugs + * + * S3 objects are downloaded completely to answer this request. + */ + virtual bool exists(std::string_view uri) = 0; + /** * Download a file, returning its contents through a source. Will not return * before the transfer has fully started, ensuring that any errors thrown by diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index f54e799c9..f3efa8124 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -114,15 +114,8 @@ protected: checkEnabled(); try { - FileTransferRequest request{makeURI(path)}; - request.head = true; - getFileTransfer()->enqueueDownload(request).get(); - return true; + return getFileTransfer()->exists(makeURI(path)); } catch (FileTransferError & e) { - /* S3 buckets return 403 if a file doesn't exist and the - bucket is unlistable, so treat 403 as 404. */ - if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden) - return false; maybeDisable(); throw; }