Skip to content

Commit c08722c

Browse files
committed
curlFileTransfer: Lazily create activity and set startTime
There can be a long time between the creation of `TransferItem` and the start of the curl download, which can lead to misleading download durations and progress bar status. So now we create the `Activity` and update `startTime` when curl actually starts the download.
1 parent ae2b69f commit c08722c

File tree

1 file changed

+35
-9
lines changed

1 file changed

+35
-9
lines changed

src/libstore/filetransfer.cc

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ struct curlFileTransfer : public FileTransfer
5353
curlFileTransfer & fileTransfer;
5454
FileTransferRequest request;
5555
FileTransferResult result;
56-
Activity act;
56+
std::unique_ptr<Activity> _act;
5757
bool done = false; // whether either the success or failure function has been called
5858
Callback<FileTransferResult> callback;
5959
CURL * req = 0;
@@ -98,12 +98,6 @@ struct curlFileTransfer : public FileTransfer
9898
Callback<FileTransferResult> && callback)
9999
: fileTransfer(fileTransfer)
100100
, request(request)
101-
, act(*logger,
102-
lvlTalkative,
103-
actFileTransfer,
104-
fmt("%s '%s'", request.verb(/*continuous=*/true), request.uri),
105-
{request.uri.to_string()},
106-
request.parentAct)
107101
, callback(std::move(callback))
108102
, finalSink([this](std::string_view data) {
109103
if (errorSink) {
@@ -301,9 +295,29 @@ struct curlFileTransfer : public FileTransfer
301295
return ((TransferItem *) userp)->headerCallback(contents, size, nmemb);
302296
}
303297

298+
/**
299+
* Lazily start an `Activity`. We don't do this in the `TransferItem` constructor to avoid showing downloads
300+
* that are only enqueued but not actually started.
301+
*/
302+
Activity & act()
303+
{
304+
if (!_act) {
305+
_act = std::make_unique<Activity>(
306+
*logger,
307+
lvlTalkative,
308+
actFileTransfer,
309+
fmt("%s '%s'", request.verb(/*continuous=*/true), request.uri),
310+
Logger::Fields{request.uri.to_string()},
311+
request.parentAct);
312+
// Reset the start time to when we actually started the download.
313+
startTime = std::chrono::steady_clock::now();
314+
}
315+
return *_act;
316+
}
317+
304318
int progressCallback(curl_off_t dltotal, curl_off_t dlnow) noexcept
305319
try {
306-
act.progress(dlnow, dltotal);
320+
act().progress(dlnow, dltotal);
307321
return getInterrupted();
308322
} catch (nix::Interrupted &) {
309323
assert(getInterrupted());
@@ -380,6 +394,13 @@ struct curlFileTransfer : public FileTransfer
380394
return ((TransferItem *) clientp)->seekCallback(offset, origin);
381395
}
382396

397+
static int resolverCallbackWrapper(void *, void *, void * clientp) noexcept
398+
{
399+
// Create the `Activity` associated with this download.
400+
((TransferItem *) clientp)->act();
401+
return 0;
402+
}
403+
383404
void unpause()
384405
{
385406
/* Unpausing an already unpaused transfer is a no-op. */
@@ -497,6 +518,11 @@ struct curlFileTransfer : public FileTransfer
497518
}
498519
#endif
499520

521+
// This seems to be the earliest libcurl callback that signals that the download is happening, so we can
522+
// call act().
523+
curl_easy_setopt(req, CURLOPT_RESOLVER_START_FUNCTION, resolverCallbackWrapper);
524+
curl_easy_setopt(req, CURLOPT_RESOLVER_START_DATA, this);
525+
500526
result.data.clear();
501527
result.bodySize = 0;
502528
}
@@ -545,7 +571,7 @@ struct curlFileTransfer : public FileTransfer
545571
if (httpStatus == 304 && result.etag == "")
546572
result.etag = request.expectedETag;
547573

548-
act.progress(result.bodySize, result.bodySize);
574+
act().progress(result.bodySize, result.bodySize);
549575
done = true;
550576
callback(std::move(result));
551577
}

0 commit comments

Comments
 (0)