From e4cecabbf38353e27be0f3b43858a15ec92d5022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 20 Dec 2020 12:36:32 +0100 Subject: [PATCH] UWP: Work around another race condition in StorageFileLoader. It really needs a rewrite and better error handling but that's for later. --- Common/LogManager.cpp | 4 +- UWP/StorageFileLoader.cpp | 95 ++++++++++++++++++++++----------------- 2 files changed, 58 insertions(+), 41 deletions(-) diff --git a/Common/LogManager.cpp b/Common/LogManager.cpp index d518df7a99ae..1a7c6cffaa03 100644 --- a/Common/LogManager.cpp +++ b/Common/LogManager.cpp @@ -304,8 +304,10 @@ void FileLogListener::Log(const LogMessage &message) { } void OutputDebugStringLogListener::Log(const LogMessage &message) { + char buffer[4096]; + snprintf(buffer, sizeof(buffer), "%s %s %s", message.timestamp, message.header, message.msg.c_str()); #if _MSC_VER - OutputDebugStringUTF8(message.msg.c_str()); + OutputDebugStringUTF8(buffer); #endif } diff --git a/UWP/StorageFileLoader.cpp b/UWP/StorageFileLoader.cpp index 81c6dde3ae6c..36437827c00a 100644 --- a/UWP/StorageFileLoader.cpp +++ b/UWP/StorageFileLoader.cpp @@ -1,5 +1,7 @@ #include "pch.h" #include "ppltasks.h" + +#include "Common/Log.h" #include "Common/File/FileUtil.h" #include "Common/File/DirListing.h" #include "Common/Thread/ThreadUtil.h" @@ -11,64 +13,77 @@ using namespace Concurrency; using namespace Windows::Storage; using namespace Windows::Storage::Streams; +// Not sure how necessary this one is. static std::mutex initMutex; StorageFileLoader::StorageFileLoader(Windows::Storage::StorageFile ^file) { + active_ = false; file_ = file; path_ = FromPlatformString(file_->Path); + INFO_LOG(IO, "StorageFileLoader - launching thread"); thread_.reset(new std::thread([this]() { this->threadfunc(); })); + + // Before we proceed, we need to block until the thread has found the size. + // Hacky way: + while (size_ < 0) { + Sleep(10); + } } StorageFileLoader::~StorageFileLoader() { + INFO_LOG(IO, "StorageFileLoader destructor"); { std::unique_lock lock(mutex_); + INFO_LOG(IO, "StorageFileLoader - setting active to false"); active_ = false; operationRequested_ = false; - cond_.notify_all(); + cond_.notify_one(); } + INFO_LOG(IO, "StorageFileLoader - joining with thread"); thread_->join(); } void StorageFileLoader::threadfunc() { setCurrentThreadName("StorageFileLoader"); - initMutex.lock(); - auto opentask = create_task(file_->OpenReadAsync()).then([this](IRandomAccessStreamWithContentType ^stream) { - stream_ = stream; - active_ = true; - }); - - try { - opentask.wait(); - } - catch (const std::exception& e) { - operationFailed_ = true; - // TODO: What do we do? - const char *what = e.what(); - INFO_LOG(SYSTEM, "%s", what); - } - catch (Platform::COMException ^e) { + { + std::unique_lock lock(initMutex); + _assert_(!active_); + auto opentask = create_task(file_->OpenReadAsync()).then([this](IRandomAccessStreamWithContentType ^stream) { + stream_ = stream; + active_ = true; + INFO_LOG(IO, "StorageFileLoader: active"); + }); + + try { + opentask.wait(); + INFO_LOG(IO, "StorageFileLoader: first wait finished"); + } catch (const std::exception& e) { + operationFailed_ = true; + // TODO: What do we do? + const char *what = e.what(); + INFO_LOG(SYSTEM, "%s", what); + } catch (Platform::COMException ^e) { - } + } - auto sizetask = create_task(file_->GetBasicPropertiesAsync()).then([this](Windows::Storage::FileProperties::BasicProperties ^props) { - size_ = props->Size; - }); - try { - sizetask.wait(); - } - catch (const std::exception& e) { - const char *what = e.what(); - INFO_LOG(SYSTEM, "%s", what); - } - catch (Platform::COMException ^e) { - std::string what = FromPlatformString(e->ToString()); - INFO_LOG(SYSTEM, "%s", what.c_str()); + auto sizetask = create_task(file_->GetBasicPropertiesAsync()).then([this](Windows::Storage::FileProperties::BasicProperties ^props) { + size_ = props->Size; + INFO_LOG(IO, "StorageFileLoader: Got size: %d", (int)size_); + }); + try { + sizetask.wait(); + INFO_LOG(IO, "StorageFileLoader: second wait finished"); + } catch (const std::exception& e) { + const char *what = e.what(); + INFO_LOG(SYSTEM, "%s", what); + } catch (Platform::COMException ^e) { + std::string what = FromPlatformString(e->ToString()); + INFO_LOG(SYSTEM, "%s", what.c_str()); + } } - initMutex.unlock(); - std::unique_lock lock(mutex_); while (active_) { if (!operationRequested_) { @@ -134,17 +149,15 @@ std::string StorageFileLoader::Extension() { } void StorageFileLoader::EnsureOpen() { - // UGLY! - while (!thread_) - Sleep(100); while (size_ == -1) - Sleep(100); + Sleep(50); } size_t StorageFileLoader::ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags) { EnsureOpen(); - if (operationRequested_ || responseAvailable_) - Crash(); + _assert_(!operationRequested_); + _assert_(!responseAvailable_) + { std::unique_lock lock(mutex_); operation_.type = OpType::READ_AT; @@ -160,6 +173,9 @@ size_t StorageFileLoader::ReadAt(s64 absolutePos, size_t bytes, size_t count, vo while (!responseAvailable_) { condResponse_.wait(responseLock); } + INFO_LOG(IO, "StorageFileLoader: Got response"); + // still under mutexResponse_ lock here. + responseAvailable_ = false; if (operationFailed_) { return 0; } @@ -168,7 +184,6 @@ size_t StorageFileLoader::ReadAt(s64 absolutePos, size_t bytes, size_t count, vo Platform::Array ^bytearray = ref new Platform::Array((unsigned int)len); rd->ReadBytes(bytearray); memcpy(data, bytearray->Data, len); - responseAvailable_ = false; response_.buffer = nullptr; return len / bytes; }