5
\$\begingroup\$

The problem I am trying to solve as an exercise is as follows:

  • I have a UI running in the main thread, specifically a list view with many items. Each item represents a file that the user can download. To do so, you need to double-click on the item and the download process should start.
  • I don't want the download process to block the UI thread, so the only solution I see is to run the download process in a separate thread (separate from the main thread in which the UI loop runs).
  • However, the user might want to cancel the download.

I came up with the following design:

  • if you double-click on an item, start the download thread by passing to the function executed by the tread the caller (the list view in my case) and some additional info such as the item index in the list view and the filename.
  • When the thread starts it registers itself by calling the RegisterThread method on the List instance that started the thread. The thread enters the download loop (use something like recv to get batches of data until the download is complete) but on each iteration of the loop it asks list view whether it should keep running by calling list->ShouldThreadStop.
  • I am using 2 maps in the List class to keep track of the item index-thread id relation and the thread id and bool flag (that indicates whether a given thread should keep running) relation. When the thread ends (end of download or requested to stop), it needs to call a Deregister method on List to remove appropriate entries in the maps.

In the review process from peers and C++ gurus I am particular interested in the following points:

  • This is a rather common feature (a list of files to download). Do you think the solution of creating a separate thread to handle the download process is the most appropriate approach?
  • Is my design for registering/deregistering the thread so that the calling instance (list in my case) can stop the thread the best approach too? Is there a better pattern for this?
  • Do you see issues with this design (besides that, I don't check bounds here for the index and don't necessarily handle cases where I look up in the map without checking if the entry exists). I am more interested in the bigger picture. While a bit involved, there's no simpler, more "correct" (assuming there's something fundamentally wrong with the chosen approach)? But this is why I am posting here).
  • One thing that worries me is what happens if a user deletes an item in the list corresponding to a file that's being downloaded. But I can also determine whether a thread runs for that item and cancel it if needed. The thread itself should not rely on data that would be in the list while running (passing the filename as a reference might be an issue, so I can pass it by value).
  • As a side note, am I using the mutex right here? Is there a better way?

I value and appreciate you spending time on this code and sharing your knowledge and experience.

Here is the code:

//  clang++ -std=c++2b -pthread -o list_view list_view.cc
#include <iostream>
#include <vector>
#include <thread>
#include <functional>
#include <string>
#include <map>
#include <chrono>
#include <atomic>
#include <mutex>
#include <csignal>

class List;
void DownloadFile(List* list, uint32_t index, const std::string& filename);


class List {
public:
    List() {
        filenames_.insert(filenames_.begin(), {"a.bin", "b.bin", "c.bin", "d.bin"});
    }
    void DoubleClick(uint32_t index) {
        std::thread th(DownloadFile, this, index, filenames_[index]);
        th.detach();
    }
    void RegisterThread(uint32_t index, std::thread::id id) {
        std::lock_guard<std::mutex> lock(mutex_);
        //if (should_stop_map_.find(id) != should_stop_map_.end())
        should_stop_map_[id] = false;
        index_to_thread_id_[index] = id;
    }
    void DeregisterThread(int index) {
        std::lock_guard<std::mutex> lock(mutex_);
        std::thread::id id = index_to_thread_id_[index];
        should_stop_map_.erase(id);
        index_to_thread_id_.erase(index);
    }
    bool ShouldThreadStop(std::thread::id id) {
        std::lock_guard<std::mutex> lock(mutex_);
        if (should_stop_map_.find(id) != should_stop_map_.end()) {
            return should_stop_map_[id];
        }
        return false;
    }
    void StopThreadByIndex(uint32_t index) {
        std::lock_guard<std::mutex> lock(mutex_);
        std::thread::id id = index_to_thread_id_[index];
        if (should_stop_map_.find(id) != should_stop_map_.end())
            should_stop_map_[id] = true;
    }
    std::vector<std::string> filenames_;
    std::map<std::thread::id, std::atomic<bool>> should_stop_map_;
    std::map<uint32_t, std::thread::id> index_to_thread_id_;
    std::mutex mutex_;
};

void DownloadFile(List* list, uint32_t index, const std::string& filename) {
    list->RegisterThread(index, std::this_thread::get_id());
    std::cerr << "Start to download: " << filename << std::endl;
    int i = 0;
    while (!list->ShouldThreadStop(std::this_thread::get_id()) && i++ < 10) {
        // to simulate some data received from a server
        std::this_thread::sleep_for(std::chrono::milliseconds(300));
    }
    list->DeregisterThread(index);
    std::cerr << "I am either done or was stoped. Progress: " << i << ", index: " << index << std::endl;
}

List list;

void SignalHandler(int singint) {
    std::cerr << list.should_stop_map_.size() << " " << 
        list.index_to_thread_id_.size() << std::endl;
    exit(singint);
}

int main() {

    signal(SIGINT, SignalHandler);

    list.DoubleClick(2);
    std::this_thread::sleep_for(std::chrono::milliseconds(300));
    list.DoubleClick(0);
    std::this_thread::sleep_for(std::chrono::milliseconds(300));
    list.StopThreadByIndex(2);
    
    for (;;) {}

    return 0;
 }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. What you can do is create a follow up question with a link back to this question. \$\endgroup\$
    – pacmaninbw
    Jul 13, 2023 at 12:45

2 Answers 2

3
\$\begingroup\$

I’m going to start the review by answering the questions briefly, then afterwards, expand a bit. So…:

Questions

Creating a separate thread to handle the download process is the most appropriate approach?

No.

Remember that std::thread is not really the same as a “thread” in most other modern languages. In most languages, a “thread” is just an abstraction that may or may not map to a concrete “thing” (operating system thread or processor thread or whatever). These are sometimes called “green threads”. A lot of times, the language’s run-time creates a fixed number of “real” threads in a thread pool, and then the “green threads” you create in the code are scheduled on that pool as needed. But a std::thread is a literal, real, operating system thread.

That has some important implications. In most modern languages, because “green threads” are not real threads, they can be pretty lightweight. You can make millions of threads in Haskell or Python (using CPython under the hood at least), and your computer will shrug and keep on trucking… because there are only, say, 4 or 8 real threads, and those millions of “threads” are just tasks in a queue that get bounced around those real threads. But in C++, if you made millions of std::thread… you will very likely bring your system to its knees.

Thus, making a std::thread for each task is… not a great idea.

std::thread is meant to be a very low-level tool. You generally shouldn’t be using it (directly) in higher-level code… like UI code. You should be using higher-level abstractions, like tasks. The standard library has std::packaged_task for that. @G.Sliepen also mentioned using std::promise and std::future directly which… well, I mean, they’re kinda mid-level abstractions. But, generally, the standard library’s concurrency facilities are still very poor, and primarily low-level, building block stuff. That’s because we’re waiting on executors (or something like them). The idea is that in the future, you will be able to create a thread pool, get a scheduler for that thread pool, then schedule your tasks (with std::packaged_task or whatever) using that scheduler. All the juggling and scheduling of those tasks will be handled automatically.

But that’s the future. In the now, you’ll pretty much have to roll most of that yourself. More on that later.

Registering/deregistering the thread so that the calling instance can stop the thread the best approach too? Is there a better pattern for this?

Well, the pattern is okay. Your implementation is a bit… not good.

@G.Sliepen already mentioned that detaching threads is usually a bad idea. Indeed, I would suggest using std::thread at all is a bad idea. If anything you should use std::jthread.

It’s a little back-assward to throw away the thread handle, and instead give the thread a handle to the caller and expect it to check back on whether it should stop or what work it should be doing, not to mention doing all other management tasks. Ideally, you want your tasks (or threads) to be self-contained, and responsible only for their own stuff. This jibes with your intuition as well.

(As an aside, I generally suggest that if you find yourself making a design that involves manually “registering” and “un-registering” stuff… you really should stop and rethink that design. Not always… but… it is a bit of a design smell. At the very least, a function named DeregisterThread is pretty much a flashing red light with an alarm klaxon, and a voice repeating: “R-A-I-I… R-A-I-I…”)

Consider instead a design that keeps a map of download tasks, where each task is self-contained. That, in essence, makes each task “self-registering”, simply by virtue of being an item in the map. I’ll just use the filename as the key, but you could use a list index (but that’s brittle!) or something else.

auto download_file(std::stop_token stop_token, std::string const& filename)
{
    // Presumably safe to use a reference to the filename, because the list
    // should outlive the task... but do be careful, because, for example,
    // if you mess with the vector of filenames by adding/removing/reordering
    // stuff, that could be a problem!
    //
    // If it might be a problem, just take it by value.

    std::cerr << "Start to download: " << filename << "\n";

    auto i = 0
    while (not stop_token.stop_requested() and i++ < 10)
        std::this_thread::sleep_for(std::chrono::milliseconds(300));

    std::cerr << "I am either done or was stoped. Progress: " << i << ", filename: " << filename << "\n";
}

class list
{
    std::vector<std::string> filenames_;
    std::unordered_map<std::string, std::jthread> tasks_;

    // Everything cleans up perfectly, automatically. jthread is magical!
    //
    // Note though, this will cancel any unfinished downloads. If you want
    // them to complete, you need a destructor that joins all unfinished
    // task threads.
    ~list() = default;

    auto double_click(std::size_t index) -> void
    {
        // do the requisite checks, of course:
        //   * check the index is valid...
        //   * check the filename isn’t already being downloaded...
        //   * etc....

        auto const& filename = filenames_[index];
        tasks[filename] = std::jthread{download_file, std::cref(filename)};
    }

    auto stop_thread_by_index(std::size_t index) -> void
    {
        // do the requisite checks, of course:
        //   * check the index is valid...
        //   * check the filename is being downloaded...
        //   * etc....

        tasks_[filename_[index]].request_stop();
    }
};

Do you see issues with this design?

Aside from the stuff mentioned elsewhere, I think you have separation of responsibilities issues going on.

If this class is supposed to represent a UI list, then it shouldn’t also have a responsibility to be a download manager. The list class should have one responsibility: maintaining its list. It should keep track of the items in the list, responding to UI events (click, double-click, drag, etc.) by adding/removing/reordering its items… and that’s about it. It could (and should) support hooking callbacks into events, so you could take a list and have it initiate a download on double-click… but it shouldn’t, itself, have those extra responsibilities.

And, then, also, your download manager shouldn’t also have the responsibilities of being a task scheduler. It should support adding a download… cancelling a download… and maybe setting/resetting download priorities and getting the current state and progress of a download. But managing concurrency? That should be something else’s responsibility (specifically, a task scheduler).

So, for example, you probably want:

  • a task manager… which internally holds a thread pool and a list of tasks
  • a download manager… which takes download requests, and turns them into tasks that get passed to the task manager;
  • and then a UI list class… which takes UI events and turns them into download requests.

So, like:

// Abstract task:
class task
{
    std::stop_token _stop_token;

public:
    virtual ~task() = default;

    // Cancellation support.
    auto set_stop_token(std::stop_token stop_token) noexcept { _stop_token = stop_token; }
    auto get_stop_token() const noexcept { return _stop_token; }

    virtual auto run() = 0;
};

// Task scheduler:
class task_scheduler
{
    std::vector<std::jthread> _threads;

    // If you had a proper concurrent queue, which you should, you wouldn't
    // need the mutex. (A standard concurrent queue is forthcoming.)
    std::unique_ptr<std::mutex> _task_list_mutex;
    std::unique_ptr<std::queue<std::shared_ptr<task>>> _task_list;

    static auto thread_function(
        std::stop_token stop_token,
        std::mutex& task_list_mutex,
        std::queue<std::shared_ptr<task>>& task_list)
    {
        while (not stop_token.stop_requested())
        {
            auto const task = []()
            {
                auto lock = std::scoped_lock(task_lisk_mutex);

                // This very simple scheduler just takes the next task in
                // order.
                //
                // A more sophisticated implementation may use priorities,
                // and may identify different types of tasks for different
                // threads, or may use thread affinities... whatever.
                if (not task_list.empty())
                {
                    auto task = task_list.front();
                    task_list.pop();
                    return task;
                }

                return std::shared_ptr<task>{};
            }();

            if (task)
            {
                task->set_stop_token(stop_token);
                task->run();
            }
        }
    }

public:
    task_scheduler()
        : _task_list_mutex{std::make_unique<std::mutex>()}
        , _task_list{std::make_unique<std::queue<std::shared_ptr<task>>>()}
    {
        std::ranges::generate_n(
            std::back_inserter(_threads),
            /* figure out the number of threads you want */,
            [&mutex = *_task_list_mutex, &tasks = *_task_list] ()
            {
                return std::jthread{thread_function, std::ref(mutex), std::ref(tasks)};
            }
        );
    }

    // By default, any unfinished and not started tasks will be cancelled or
    // discared. If you want all tasks to finish, you need to write a
    // destructor that waits for the task queue to be empty, and all threads
    // in the pool to join.
    ~task_scheduler() = default;

    // Note: As written this class behaves properly. It won’t allow copies,
    // it can be safely moved.

    auto schedule(std::shared_ptr<task> task)
    {
        auto lock = std::scoped_lock(*_task_list_mutex);
        _task_list->push(std::move(task));
    }
};

class download_manager
{
    class download_task : public task
    {
        std::string _filename;

    public:
        explicit download_task(std::string filename)
            : _filename{std::move(filename)}
        {}

        auto run() -> void override
        {
            auto const stop_token = get_stop_token();

            // Might need an iostreams mutex if you want to write to std::cerr
            // concurrently.

            std::cerr << "Start to download: " << _filename << "\n";

            auto i = 0
            while (not stop_token.stop_requested() and i++ < 10)
                std::this_thread::sleep_for(std::chrono::milliseconds(300));

            std::cerr << "I am either done or was stoped. Progress: " << i << ", filename: " << _filename << "\n";
        }
    };

    // Note: no concurrency support, but easily added.
    std::vector<std::weak_ptr<download_task>> _tasks;

public:
    using handle_type = download_task*;

    // Note: need to delete copying.

    auto schedule_download(task_manager& scheduler, std::string filename) -> handle_type
    {
        // Clean up finished tasks:
        std::erase_if(_tasks, [](auto&& p) { return p.expired(); });

        auto task = std::make_shared<download_task>(std::move(filename));

        _tasks.emplace_back(task);

        scheduler.schedule(task);

        return task.get();
    }

    auto cancel_download(handle_type handle)
    {
        std::erase_if(_tasks, [handle](auto&& task)
        {
            auto p = task.lock();
            return (not p) or (p.get() == handle))
        });
    }
};

So your downloading list class would simply get a reference to a download manager, and in the on-double-click item event or whatever, it would do _download_manager.schedule_download(scheduler, filename). It would keep track of the handle, and if the user requests a cancellation, call cancel_download(handle). The download manager handles everything else, or passes the buck to the schedule.

(already covered)

(already covered)

As a side note, am I using the mutex right here? Is there a better way?

It doesn’t look wrong in the technical sense, but it does seem inefficient and unwise.

You shouldn’t allow threads to modify the maps. That way lies madness. That’s too many fingers in the broth. The list should be the only thing that controls those maps.

At most, the thread only needs two things: the filename, and the cancellation token (which, in your case, is an atomic bool, which is fine). There is no need to give it control over the list as well.

But the list needs to know when the download is done, and it needs to be able to cancel the download. Rather than maintaining a bunch of maps and trying to keep it in sync, you could do this:

class list
{
    // I'll just use tuples to be quick and hacky, but you should probably
    // create proper types.
    std::vector<
        std::tuple<
            std::string,        // the filename in the list
            std::unique_ptr<    // data needed only if downloading is initiated
                std::tuple<
                    std::future<void>,  // or you can keep a handle to the thread
                    std::atomic_flag    // cancellation token
                >
            >
        >
    > _data;

    static auto thread_func(std::string filename, std::promise<void> promise, std::atomic_flag& cancelled)
    {
        try
        {
            std::cerr << "Start to download: " << filename << "\n";

            auto i = 0
            while (not cancelled.test(std::memory_order::acquire) and i++ < 10)
                std::this_thread::sleep_for(std::chrono::milliseconds(300));

            std::cerr << "I am either done or was stoped. Progress: " << i << ", filename: " << filename << "\n";

            promise.set_value_at_thread_exit();
        }
        catch (...)
        {
            promise.set_exception_at_thread_exit(std::current_exception());
        }
    }

public:
    list()
    {
        for (auto&& filenames : {"a.bin", "b.bin", "c.bin", "d.bin"})
            _data.emplace_back(filename);
    }

    ~list()
    {
        for (auto&& [filename, download_data] : _data)
        {
            if (download_data)
            {
                auto&& [future, cancel] = *download_data;
                cancel.test_and_set(std::memory_order::release); // if you want to cancel in-progress downloads
                future.wait();
            }
        }
    }

    auto double_click(std::size_t index)
    {
        // As always, with any public function, do your diligence:
        //   * is index valid?
        //   * is it already downloading?
        //   * etc....

        auto&& [filename, download_data] = _data[index];

        auto temp_data = std::make_unique<std::tuple<std::future<void>, std::atomic_flag>();

        auto&& [future, cancel] = *temp_data;

        auto promise = std::promise<void>{};
        future = promise.get_future();

        auto thread = std::thread{thread_func, filename, std::move(promise), std:ref(cancel)};
        thread.detach();

        download_data = std::move(temp_data);
    }

    auto stop_thread_by_index(std::size_t index)
    {
        if (auto download_data = std::get<1>(_data[index]); download_data != nullptr)
            std::get<std::atomic_flag>(*download_data).test_and_set(std::memory_order::release);
    }
};

The list items are stored as tuples, consisting of a string and a pointer to “extra” data… in this case, data about any downloads that have been requested. Any item with a non-nullptr data pointer has a download pending, in progress, or completed. That means that there is extra, unnecessary data stored for any list item that isn’t being downloaded… but seriously, it’s just one pointer, and the amount of code is saves you with all that extra management of maps and crap, it amortizes away many times over.

Now, in the code above I’ve just used a bunch of raw types: tuples and futures and so on. You should really make proper dedicated types for each download task. Here’s an example of what a better interface might look like:


class download_task
    : public task   // inherit from a generic task interface, that can be
                    // passed to a scheduler
{
public:
    // Might actually want different observers for the request filename/URL,
    // and the target filename.
    auto filename() const -> std::string;
    auto filename_view() const noexcept -> std::string_view;

    enum class status_type
    {
        unknown,        // task is created, but not scheduled
        pending,        // task is scheduled, but not started
        starting,       // starting download (connecting, checking for space on destination, etc....)
        in_progress,    // download actually happening
        complete,       // self-explanatory
        cancelled       // self-explanatory
    };
    auto status() const noexcept -> status_type;

    struct progress_type
    {
        std::size_t current = 0;
        std::size_t total = -1;     // -1 if we don’t know the download size
                                    // (could also use optional<size_t>)
    };
    auto progress() const noexcept -> progress_type;

    auto cancel() noexcept -> void;

    auto get_future() noexcept -> std::future<void>;

    // The actual “run” function, from the task interface, would do the
    // download, periodically checking whether it has been cancelled.
};

I think that’s enough to allow anything you might want with a download function:

  • scheduling
  • cancellation
  • progress display
  • waiting on the result (with a std::future, you can use any kind of wait function, such as waiting indefinitely, waiting for a short period, etc.

Your list can hold a vector of tuples of filenames and shared (or weak) pointers to download tasks. That way you can iterate through the list to display the filenames and download status (if any), but otherwise, the downloading and scheduling are the business of other, dedicated classes.

This is, of course, not the only solution, and maybe not even the best for your specific use case. But it is certainly a lot less complex and and error-prone than holding a bunch of maps that you have to keep in sync, and allowing multiple components to modify those maps.

I would suggest you think in terms of interfaces, rather than implementations. Look at the current interface of your list class:

class List
{
public:
    List(); // fine

    void DoubleClick(uint32_t index); // fine

    void StopThreadByIndex(uint32_t index); // fine, but poorly named
                                            //
                                            // should probably be CancelDownload
                                            // or something like that

    // These have no business being in the public interface.
    //
    // At the very least, you would need to make them private and make
    // DownloadFile a friend... but even that is still ugly, because external
    // entities shouldn’t have any need to much with the list of List. I mean
    // the whole point of List is to keep a list, so it makes little sense to
    // allow other entities to do that work.
    void RegisterThread(uint32_t index, std::thread::id id);
    void DeregisterThread(int index);

    // There is no reason any external entity should need this information.
    bool ShouldThreadStop(std::thread::id id);
};

That’s not a UI list interface. It is, if anything, a download manager class interface. That’s your problem domain whispering to you that you need to restructure your design.

\$\endgroup\$
5
  • \$\begingroup\$ I will add (limited size answer) that your point on threads being core threads in C++ is good. Initially my idea was to mimic downloads in tools like Chrome or google drive where multiple files can be downloaded at once. So easy route was: 1 thread / file. But it seems overkilled indeed: I should run just 1 single bg thread instead. And then find a mechanism/pattern on the client-server side to serve multiple files at once. Love the fact that you used views, suggested std::packaged_task and std::jthread::stop_requested. Learned a lot with both answers. \$\endgroup\$ Jul 16, 2023 at 12:39
  • \$\begingroup\$ "If this class is supposed to represent a UI list, then it shouldn’t also have a responsibility to be a download manager." And to rephrase (deleted) comment, that's why I was mentioning (per @G. Sliepen suggestion) that I shall implement for this a MVC pattern where the View would handle the mouse events, etc. and the controller would essentially be what you call the Download Manager. I will show this progress / improvement for that in a separate post as I was recommended not to edit the original questions with edits. \$\endgroup\$ Jul 16, 2023 at 12:43
  • \$\begingroup\$ Love both answers (can't say thanks enough) but accepted yours as you provide some implementations for solutions such as a way of getting a task manager, download manager and the UI view. Really extremely grateful for your time and knowledge sharing generosity. \$\endgroup\$ Jul 16, 2023 at 12:46
  • \$\begingroup\$ ps what's the benefit of for (auto&& filenames : {"a.bin", "b.bin", "c.bin", "d.bin"}) here vs for (auto& filenames : {"a.bin", "b.bin", "c.bin", "d.bin"})? \$\endgroup\$ Jul 16, 2023 at 15:12
  • \$\begingroup\$ auto& usually works, but there are cases where it will fail, like for (auto& v : vector<bool>{true, false}). auto&& always works. I teach that you should use auto&& for read-only access by default, auto const& for read-only access and you want to make that explicit, auto if you mean to copy each value before using it, and auto& only when you mean to modify the values in the source range. (But in general, you should not use for loops anyway, you should use algorithms. In that case, transform… but really, you should just construct the vector with the data directly.) \$\endgroup\$
    – indi
    Jul 16, 2023 at 18:21
3
\$\begingroup\$

Don't detach threads

You should almost never detach threads. By detaching them you lose control over them. In particular, you might get lifetime issues: for example, you pass this to the thread created in List::DoubleClick(), but what if the List object goes out of scope before the thread finishes? When the thread calls list->DeregisterThread(), at best you will get a crash, at worst your program will start behaving incorrectly.

Threads should not register themselves with a List object, the List object should own threads. Since you want to associate a thread with a filename in the list, couple those together:

class List {
    …
    struct Entry {
        std::string filename;
        std::thread thread;
    };

    std::vector<Entry> entries_;
};

Then to start a thread you can:

void DoubleClick(std::size_t index) {
    entries_[index] = std::thread(DownloadFile, this, index);
}

And if you make DownloadFile() a static member function of List, it can access entries_. So that also allows you to add more state to struct Entry, for example you can add a should_stop member variable to it.

Make sure you join() any threads that have finished and/or when a List is destructed. If you can use C++20, I recommend that you use std::jthread instead, which takes care of that automatically.

Modifying the list

If you need to add, remove or modify entries in the list, this is going to be a problem for threads that have already started. The index they have might no longer be valid. You must then find some other way to ensure data used by threads is not invalidated. There are many ways you can do that. Some ideas:

  • Use std::vector<std::shared_ptr<Entry>> so Entrys can be passed to a thread without worrying about it being moved/deleted from the list.
  • Use a separate container for storing threads and their should_stop variables.
  • Just prevent modifying the list while there are active threads.

Alternatives to std::thread

Instead of std::thread you might also use std::async(). It can return a std::future holding the result of the async work. For example, you could make DownloadFile() return a std::vector<std::byte> containing the file.

What if you double-click many times?

What if you double-click the same item multiple times? Your RegisterThread() will then forget all but the last thread that registered itself, but there still will be multiple threads trying to download the same file. You should have some way to detect this.

Even if I don't double-click the same item, what if I double-click hundreds of different items? If all files come from the same site, the site's owner might not like so many concurrent requests. Instead of one thread per file, you might want to create a queue of download requests, and only have one or a few threads downloading at the same time. Have a look at thread pool implementations for how to do this.

Interaction with the UI

The code you have posted does not have a UI at all, but you might need to have some interaction between your list, threads and the UI. Think about showing download progress, signalling that a download is finished or encountered an error, and so on. There is more to it than just starting and stopping a download.

You might want to use the model-view-controller design pattern to separate the various aspects of your GUI program. Your class List as it is now would be the model. However, I would rename DoubleClick() to Download(): how you initiate the download exactly is a problem of the controller, the model doesn't need to know whether that is via single-click, double-click or pressing a key on the keyboard.

\$\endgroup\$
3
  • \$\begingroup\$ @G Sliepen. Thanks a lot for all your suggestions. Regarding the last point, yes, that's the idea but not shown here for brevity. Regarding your first suggestion, It makes sense but would struct entry { std::string, std::thread } be good If the list has thousands of items and they have all be downloaded already; then std::thread would be unused. Ok I'd agree that's probably marginal (16 bytes to hold a thread). \$\endgroup\$ Jul 13, 2023 at 8:36
  • \$\begingroup\$ @G Sliepen: "And if you make DownloadFile() a static member function of List, it can access entries_" making the DownloadFile static ok but it wouldn't be given access to the non-static member variables from the class? Did you have something else in mind here (like passing this as an argument to DownloadFile? \$\endgroup\$ Jul 13, 2023 at 10:01
  • \$\begingroup\$ Yes, you still need to pass this. \$\endgroup\$
    – G. Sliepen
    Jul 13, 2023 at 11:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.