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.
