From 8d2458af16518e3d3e9951841d9f1f3fa6844fb6 Mon Sep 17 00:00:00 2001 From: adazem009 <68537469+adazem009@users.noreply.github.com> Date: Sun, 12 Jan 2025 22:38:24 +0100 Subject: [PATCH 1/2] ProjectDownloader: Fix asset downloading getting stuck when cancelled --- src/internal/projectdownloader.cpp | 21 +++------------------ src/internal/projectdownloader.h | 3 +-- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/internal/projectdownloader.cpp b/src/internal/projectdownloader.cpp index c334da55..28a618d5 100644 --- a/src/internal/projectdownloader.cpp +++ b/src/internal/projectdownloader.cpp @@ -17,14 +17,11 @@ static const std::string ASSET_PREFIX = "https://assets.scratch.mit.edu/internal static const std::string ASSET_SUFFIX = "/get"; #define CHECK_CANCEL() \ - m_cancelMutex.lock(); \ if (m_cancel) { \ m_downloadedAssetCount = 0; \ - m_cancelMutex.unlock(); \ std::cout << "Download aborted!" << std::endl; \ return false; \ - } \ - m_cancelMutex.unlock() + } ProjectDownloader::ProjectDownloader(IDownloaderFactory *downloaderFactory) : m_downloaderFactory(downloaderFactory) @@ -38,9 +35,7 @@ ProjectDownloader::ProjectDownloader(IDownloaderFactory *downloaderFactory) : bool ProjectDownloader::downloadJson(const std::string &projectId) { - m_cancelMutex.lock(); m_cancel = false; - m_cancelMutex.unlock(); // Get project token std::cout << "Fetching project info of " << projectId << std::endl; @@ -89,9 +84,7 @@ bool ProjectDownloader::downloadJson(const std::string &projectId) bool ProjectDownloader::downloadAssets(const std::vector &assetIds) { - m_cancelMutex.lock(); m_cancel = false; - m_cancelMutex.unlock(); auto count = assetIds.size(); // unsigned int threadCount = std::thread::hardware_concurrency(); @@ -119,20 +112,14 @@ bool ProjectDownloader::downloadAssets(const std::vector &assetIds) // Download assets auto f = [this, count](std::shared_ptr downloader, int index, const std::string &id) { - m_cancelMutex.lock(); - if (m_cancel) return; - m_cancelMutex.unlock(); - bool ret = downloader->download(ASSET_PREFIX + id + ASSET_SUFFIX); if (!ret) { std::cerr << "Failed to download asset: " << id << std::endl; - m_cancelMutex.lock(); m_cancel = true; - m_cancelMutex.unlock(); return; } @@ -178,7 +165,7 @@ bool ProjectDownloader::downloadAssets(const std::vector &assetIds) m_assetsMutex.lock(); - if (m_downloadedAssetCount != lastCount) { + if (m_downloadedAssetCount != lastCount && !m_cancel) { std::cout << "Downloaded assets: " << m_downloadedAssetCount << " of " << count << std::endl; lastCount = m_downloadedAssetCount; } @@ -197,7 +184,7 @@ bool ProjectDownloader::downloadAssets(const std::vector &assetIds) threads.erase(index); } - if (done) { + if (done || m_cancel) { for (auto &[index, info] : threads) info.first.join(); @@ -212,9 +199,7 @@ bool ProjectDownloader::downloadAssets(const std::vector &assetIds) void ProjectDownloader::cancel() { - m_cancelMutex.lock(); m_cancel = true; - m_cancelMutex.unlock(); m_downloadedAssetCount = 0; } diff --git a/src/internal/projectdownloader.h b/src/internal/projectdownloader.h index 15b47472..07893458 100644 --- a/src/internal/projectdownloader.h +++ b/src/internal/projectdownloader.h @@ -36,8 +36,7 @@ class ProjectDownloader : public IProjectDownloader std::vector m_assets; std::mutex m_assetsMutex; std::atomic m_downloadedAssetCount = 0; - bool m_cancel = false; - std::mutex m_cancelMutex; + std::atomic m_cancel = false; sigslot::signal m_downloadProgressChanged; }; From a7fb0cee51d8696467f3158af74c830af1fdb6b8 Mon Sep 17 00:00:00 2001 From: adazem009 <68537469+adazem009@users.noreply.github.com> Date: Sun, 12 Jan 2025 22:42:39 +0100 Subject: [PATCH 2/2] Project: Add stopLoading() method --- include/scratchcpp/project.h | 1 + src/project.cpp | 10 ++++++++ src/project_p.cpp | 43 ++++++++++++++++++++++++++++++++++- src/project_p.h | 2 ++ test/project/project_test.cpp | 14 ++++++++++++ 5 files changed, 69 insertions(+), 1 deletion(-) diff --git a/include/scratchcpp/project.h b/include/scratchcpp/project.h index 6f1c455c..10a3e365 100644 --- a/include/scratchcpp/project.h +++ b/include/scratchcpp/project.h @@ -29,6 +29,7 @@ class LIBSCRATCHCPP_EXPORT Project Project(const Project &) = delete; bool load(); + void stopLoading(); void start(); void run(); diff --git a/src/project.cpp b/src/project.cpp index 221d8f1a..80bac15b 100644 --- a/src/project.cpp +++ b/src/project.cpp @@ -1,8 +1,10 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include "project_p.h" +#include "internal/projectdownloader.h" using namespace libscratchcpp; @@ -27,6 +29,14 @@ bool Project::load() return impl->load(); } +/*! Cancels project loading if loading in another thread. */ +void Project::stopLoading() +{ + std::cout << "Aborting project loading..." << std::endl; + impl->stopLoading = true; + impl->downloader->cancel(); +} + /*! * Calls all "when green flag clicked" blocks. * \note Nothing will happen until run() or frame() is called. diff --git a/src/project_p.cpp b/src/project_p.cpp index e3450fb4..c8c40e20 100644 --- a/src/project_p.cpp +++ b/src/project_p.cpp @@ -47,6 +47,8 @@ bool ProjectPrivate::load() bool ProjectPrivate::tryLoad(IProjectReader *reader) { + stopLoading = false; + // Load from URL ProjectUrl url(fileName); @@ -57,8 +59,18 @@ bool ProjectPrivate::tryLoad(IProjectReader *reader) return false; } + if (stopLoading) { + loadingAborted(); + return false; + } + bool ret = reader->loadData(downloader->json()); + if (stopLoading) { + loadingAborted(); + return false; + } + if (!ret) return false; @@ -91,7 +103,14 @@ bool ProjectPrivate::tryLoad(IProjectReader *reader) } // Download assets - if (!downloader->downloadAssets(assetNames)) { + ret = downloader->downloadAssets(assetNames); + + if (stopLoading) { + loadingAborted(); + return false; + } + + if (!ret) { std::cerr << "Failed to download the project assets." << std::endl; return false; } @@ -113,7 +132,18 @@ bool ProjectPrivate::tryLoad(IProjectReader *reader) return false; } + if (stopLoading) { + loadingAborted(); + return false; + } + bool ret = reader->load(); + + if (stopLoading) { + loadingAborted(); + return false; + } + if (!ret) return false; } @@ -125,9 +155,20 @@ bool ProjectPrivate::tryLoad(IProjectReader *reader) engine->setExtensions(reader->extensions()); engine->setUserAgent(reader->userAgent()); engine->compile(); + + if (stopLoading) { + loadingAborted(); + return false; + } + return true; } +void ProjectPrivate::loadingAborted() +{ + std::cout << "Loading aborted." << std::endl; +} + void ProjectPrivate::start() { engine->start(); diff --git a/src/project_p.h b/src/project_p.h index d935835c..cb937f3a 100644 --- a/src/project_p.h +++ b/src/project_p.h @@ -21,6 +21,7 @@ struct ProjectPrivate bool load(); bool tryLoad(IProjectReader *reader); + void loadingAborted(); void start(); void run(); @@ -29,6 +30,7 @@ struct ProjectPrivate sigslot::signal &downloadProgressChanged(); std::string fileName; + std::atomic stopLoading = false; std::shared_ptr engine = nullptr; static IProjectDownloaderFactory *downloaderFactory; diff --git a/test/project/project_test.cpp b/test/project/project_test.cpp index 93f350f2..e1129238 100644 --- a/test/project/project_test.cpp +++ b/test/project/project_test.cpp @@ -115,3 +115,17 @@ TEST(LoadProjectTest, DownloadProgressChanged) EXPECT_CALL(*downloader, downloadProgressChanged).WillOnce(ReturnRef(signal)); ASSERT_EQ(&p.downloadProgressChanged(), &signal); } + +TEST(LoadProjectTest, AbortDownload) +{ + ProjectDownloaderFactoryMock factory; + auto downloader = std::make_shared(); + ProjectPrivate::downloaderFactory = &factory; + + EXPECT_CALL(factory, create()).WillOnce(Return(downloader)); + Project p; + ProjectPrivate::downloaderFactory = nullptr; + + EXPECT_CALL(*downloader, cancel()); + p.stopLoading(); +}