Merge pull request #13067 from hrydgard/download-rework

Different handling of Download lifetimes.
hrydgard · Jun 28, 2020 · 807fcf8 · 807fcf8
2 parents ed9df02 + bd8083f
commit 807fcf8
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 14 deletions.
diff --git a/ext/native/net/http_client.cpp b/ext/native/net/http_client.cpp
@@ -436,12 +436,21 @@ Download::Download(const std::string &url, const std::string &outfile)
 }
 
 Download::~Download() {
-	if (thread_.joinable())
-		thread_.join();
+	if (!joined_) {
+		FLOG("Download destructed without join");
+	}
+}
+
+void Download::Start() {
+	thread_ = std::thread(std::bind(&Download::Do, this));
 }
 
-void Download::Start(std::shared_ptr<Download> self) {
-	thread_ = std::thread(std::bind(&Download::Do, this, self));
+void Download::Join() {
+	if (joined_) {
+		ELOG("Already joined thread!");
+	}
+	thread_.join();
+	joined_ = true;
 }
 
 void Download::SetFailed(int code) {
@@ -489,11 +498,8 @@ std::string Download::RedirectLocation(const std::string &baseUrl) {
 	return redirectUrl;
 }
 
-void Download::Do(std::shared_ptr<Download> self) {
+void Download::Do() {
 	setCurrentThreadName("Downloader::Do");
-	// as long as this is in scope, we won't get destructed.
-	// yeah this is ugly, I need to think about how life time should be managed for these...
-	std::shared_ptr<Download> self_ = self;
 	resultCode_ = 0;
 
 	std::string downloadURL = url_;
@@ -542,7 +548,7 @@ void Download::Do(std::shared_ptr<Download> self) {
 std::shared_ptr<Download> Downloader::StartDownload(const std::string &url, const std::string &outfile) {
 	std::shared_ptr<Download> dl(new Download(url, outfile));
 	downloads_.push_back(dl);
-	dl->Start(dl);
+	dl->Start();
 	return dl;
 }
 
@@ -553,7 +559,7 @@ std::shared_ptr<Download> Downloader::StartDownloadWithCallback(
 	std::shared_ptr<Download> dl(new Download(url, outfile));
 	dl->SetCallback(callback);
 	downloads_.push_back(dl);
-	dl->Start(dl);
+	dl->Start();
 	return dl;
 }
 
@@ -562,6 +568,7 @@ void Downloader::Update() {
 	for (size_t i = 0; i < downloads_.size(); i++) {
 		if (downloads_[i]->Progress() == 1.0f || downloads_[i]->Failed()) {
 			downloads_[i]->RunCallback();
+			downloads_[i]->Join();
 			downloads_.erase(downloads_.begin() + i);
 			goto restart;
 		}
@@ -581,6 +588,9 @@ void Downloader::CancelAll() {
 	for (size_t i = 0; i < downloads_.size(); i++) {
 		downloads_[i]->Cancel();
 	}
+	for (size_t i = 0; i < downloads_.size(); i++) {
+		downloads_[i]->Join();
+	}
 	downloads_.clear();
 }
 

diff --git a/ext/native/net/http_client.h b/ext/native/net/http_client.h
@@ -93,9 +93,9 @@ class Download {
 	Download(const std::string &url, const std::string &outfile);
 	~Download();
 
-	// Keeps around an instance of the shared_ptr, so that it doesn't get destructed
-	// until done.
-	void Start(std::shared_ptr<Download> self);
+	void Start();
+
+	void Join();
 
 	// Returns 1.0 when done. That one value can be compared exactly - or just use Done().
 	float Progress() const { return progress_; }
@@ -138,7 +138,7 @@ class Download {
 	void SetHidden(bool hidden) { hidden_ = hidden; }
 
 private:
-	void Do(std::shared_ptr<Download> self);  // Actually does the download. Runs on thread.
+	void Do();  // Actually does the download. Runs on thread.
 	int PerformGET(const std::string &url);
 	std::string RedirectLocation(const std::string &baseUrl);
 	void SetFailed(int code);
@@ -153,6 +153,7 @@ class Download {
 	bool failed_ = false;
 	bool cancelled_ = false;
 	bool hidden_ = false;
+	bool joined_ = false;
 	std::function<void(Download &)> callback_;
 };