Skip to content

Conversation

@angt
Copy link
Collaborator

@angt angt commented Nov 29, 2025

I intentionally kept the bar simple without specifying part numbers (which ultimately don't matter much) the only thing we care about is tracking progress

I intentionally kept the bar simple without specifying part numbers
(which ultimately don't matter much) the only thing we care about is
tracking progress.

Signed-off-by: Adrien Gallouët <[email protected]>
Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I missed something, I can't find any references to std::thread in download.cpp - do we actually using more than 1 thread here?

And AFAIK, httplib::Client is single-threaded.

Edit: nevermind, see next comment

Comment on lines +491 to +492
static std::mutex mutex;
static std::map<std::thread::id, int> lines;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a local variable at first glance, but because it's static, that effectively make it a global variable.

IMO we should avoid global variable if possible. Probably a better way is to make a dedicated class to manage threads and implement the mutex as a member of the class.

@ngxson
Copy link
Collaborator

ngxson commented Nov 29, 2025

Hmm, nevermind, I found it: the multi-threads is launched by std::async which uses a thread pool under the hood.

In this case, I think it's better to introduce a mutex at common_download_file_multiple level (not global/static level)

This is the code where threads are created:

    std::vector<std::future<bool>> futures_download;
    for (auto const & item : urls) {
        futures_download.push_back(std::async(std::launch::async, [bearer_token, offline](const std::pair<std::string, std::string> & it) -> bool {
            return common_download_file_single(it.first, it.second, bearer_token, offline);
        }, item));
    }

The cleaner idea looks like this (pseudo-code):

    std::mutex mutex_progress;
    std::vector<float> progress(urls.size(), 0); // from 0.0 to 100.0
    auto update_progress = [&mutex_progress, &progress](size_t index, float value) {
        std::lock_guard<std::mutex> lock(mutex_progress);
        progress[index] = value;
        // print all progress here
    };

    std::vector<std::future<bool>> futures_download;
    for (auto const & item : urls) {
        futures_download.push_back(std::async(std::launch::async, [bearer_token, offline, update_progress](const std::pair<std::string, std::string> & it) -> bool {
            return common_download_file_single(it.first, it.second, bearer_token, offline, update_progress);
        }, item));
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants