From 597a1af85cbb86b4db2d633756280ec0df09480f Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Sat, 24 Jun 2017 21:06:18 +0300 Subject: [PATCH] Add conservative locking arond Caching/HTTP FLs Making them hopefully thread-safe. --- Core/FileLoaders/CachingFileLoader.cpp | 26 ++---- Core/FileLoaders/CachingFileLoader.h | 3 +- Core/FileLoaders/HTTPFileLoader.cpp | 105 ++++++++++++------------- Core/FileLoaders/HTTPFileLoader.h | 6 +- 4 files changed, 65 insertions(+), 75 deletions(-) diff --git a/Core/FileLoaders/CachingFileLoader.cpp b/Core/FileLoaders/CachingFileLoader.cpp index 6c9dd206cf..5e7b1b67e1 100644 --- a/Core/FileLoaders/CachingFileLoader.cpp +++ b/Core/FileLoaders/CachingFileLoader.cpp @@ -25,19 +25,16 @@ // Takes ownership of backend. CachingFileLoader::CachingFileLoader(FileLoader *backend) - : filesize_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false), prepared_(false) { + : filesize_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false) { } void CachingFileLoader::Prepare() { - if (prepared_) { - return; - } - prepared_ = true; - - filesize_ = backend_->FileSize(); - if (filesize_ > 0) { - InitCache(); - } + std::call_once(preparedFlag_, [this](){ + filesize_ = backend_->FileSize(); + if (filesize_ > 0) { + InitCache(); + } + }); } CachingFileLoader::~CachingFileLoader() { @@ -50,7 +47,6 @@ CachingFileLoader::~CachingFileLoader() { bool CachingFileLoader::Exists() { if (exists_ == -1) { - std::lock_guard guard(backendMutex_); exists_ = backend_->Exists() ? 1 : 0; } return exists_ == 1; @@ -58,7 +54,6 @@ bool CachingFileLoader::Exists() { bool CachingFileLoader::ExistsFast() { if (exists_ == -1) { - std::lock_guard guard(backendMutex_); return backend_->ExistsFast(); } return exists_ == 1; @@ -66,7 +61,6 @@ bool CachingFileLoader::ExistsFast() { bool CachingFileLoader::IsDirectory() { if (isDirectory_ == -1) { - std::lock_guard guard(backendMutex_); isDirectory_ = backend_->IsDirectory() ? 1 : 0; } return isDirectory_ == 1; @@ -78,7 +72,6 @@ s64 CachingFileLoader::FileSize() { } std::string CachingFileLoader::Path() const { - std::lock_guard guard(backendMutex_); return backend_->Path(); } @@ -92,7 +85,6 @@ size_t CachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flag size_t readSize = 0; if ((flags & Flags::HINT_UNCACHED) != 0) { - std::lock_guard guard(backendMutex_); readSize = backend_->ReadAt(absolutePos, bytes, data, flags); } else { readSize = ReadFromCache(absolutePos, bytes, data); @@ -186,9 +178,7 @@ void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool r blocksMutex_.unlock(); u8 *buf = new u8[BLOCK_SIZE]; - backendMutex_.lock(); backend_->ReadAt(cacheStartPos << BLOCK_SHIFT, BLOCK_SIZE, buf, flags); - backendMutex_.unlock(); blocksMutex_.lock(); // While blocksMutex_ was unlocked, another thread may have read. @@ -202,9 +192,7 @@ void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool r blocksMutex_.unlock(); u8 *wholeRead = new u8[blocksToRead << BLOCK_SHIFT]; - backendMutex_.lock(); backend_->ReadAt(cacheStartPos << BLOCK_SHIFT, blocksToRead << BLOCK_SHIFT, wholeRead, flags); - backendMutex_.unlock(); blocksMutex_.lock(); for (size_t i = 0; i < blocksToRead; ++i) { diff --git a/Core/FileLoaders/CachingFileLoader.h b/Core/FileLoaders/CachingFileLoader.h index 025723943c..c9f3031139 100644 --- a/Core/FileLoaders/CachingFileLoader.h +++ b/Core/FileLoaders/CachingFileLoader.h @@ -77,7 +77,6 @@ private: std::map blocks_; std::recursive_mutex blocksMutex_; - mutable std::mutex backendMutex_; bool aheadThread_; - bool prepared_; + std::once_flag preparedFlag_; }; diff --git a/Core/FileLoaders/HTTPFileLoader.cpp b/Core/FileLoaders/HTTPFileLoader.cpp index ebada0dae7..c18e7eef56 100644 --- a/Core/FileLoaders/HTTPFileLoader.cpp +++ b/Core/FileLoaders/HTTPFileLoader.cpp @@ -22,70 +22,67 @@ #include "Core/FileLoaders/HTTPFileLoader.h" HTTPFileLoader::HTTPFileLoader(const std::string &filename) - : filesize_(0), filepos_(0), url_(filename), filename_(filename), connected_(false), prepared_(false) { + : filesize_(0), filepos_(0), url_(filename), filename_(filename), connected_(false) { } void HTTPFileLoader::Prepare() { - if (prepared_) { - return; - } - prepared_ = true; + std::call_once(preparedFlag_, [this](){ + if (!client_.Resolve(url_.Host().c_str(), url_.Port())) { + // TODO: Should probably set some flag? + return; + } - if (!client_.Resolve(url_.Host().c_str(), url_.Port())) { - // TODO: Should probably set some flag? - return; - } + Connect(); + int err = client_.SendRequest("HEAD", url_.Resource().c_str()); + if (err < 0) { + Disconnect(); + return; + } - Connect(); - int err = client_.SendRequest("HEAD", url_.Resource().c_str()); - if (err < 0) { - Disconnect(); - return; - } + Buffer readbuf; + std::vector responseHeaders; + int code = client_.ReadResponseHeaders(&readbuf, responseHeaders); + if (code != 200) { + // Leave size at 0, invalid. + ERROR_LOG(LOADER, "HTTP request failed, got %03d for %s", code, filename_.c_str()); + Disconnect(); + return; + } - Buffer readbuf; - std::vector responseHeaders; - int code = client_.ReadResponseHeaders(&readbuf, responseHeaders); - if (code != 200) { - // Leave size at 0, invalid. - ERROR_LOG(LOADER, "HTTP request failed, got %03d for %s", code, filename_.c_str()); - Disconnect(); - return; - } - - // TODO: Expire cache via ETag, etc. - bool acceptsRange = false; - for (std::string header : responseHeaders) { - if (startsWithNoCase(header, "Content-Length:")) { - size_t size_pos = header.find_first_of(' '); - if (size_pos != header.npos) { - size_pos = header.find_first_not_of(' ', size_pos); + // TODO: Expire cache via ETag, etc. + bool acceptsRange = false; + for (std::string header : responseHeaders) { + if (startsWithNoCase(header, "Content-Length:")) { + size_t size_pos = header.find_first_of(' '); + if (size_pos != header.npos) { + size_pos = header.find_first_not_of(' ', size_pos); + } + if (size_pos != header.npos) { + filesize_ = atoll(&header[size_pos]); + } } - if (size_pos != header.npos) { - filesize_ = atoll(&header[size_pos]); + if (startsWithNoCase(header, "Accept-Ranges:")) { + std::string lowerHeader = header; + std::transform(lowerHeader.begin(), lowerHeader.end(), lowerHeader.begin(), tolower); + // TODO: Delimited. + if (lowerHeader.find("bytes") != lowerHeader.npos) { + acceptsRange = true; + } } } - if (startsWithNoCase(header, "Accept-Ranges:")) { - std::string lowerHeader = header; - std::transform(lowerHeader.begin(), lowerHeader.end(), lowerHeader.begin(), tolower); - // TODO: Delimited. - if (lowerHeader.find("bytes") != lowerHeader.npos) { - acceptsRange = true; - } + + // TODO: Keepalive instead. + Disconnect(); + + if (!acceptsRange) { + WARN_LOG(LOADER, "HTTP server did not advertise support for range requests."); + } + if (filesize_ == 0) { + ERROR_LOG(LOADER, "Could not determine file size for %s", filename_.c_str()); } - } - // TODO: Keepalive instead. - Disconnect(); - - if (!acceptsRange) { - WARN_LOG(LOADER, "HTTP server did not advertise support for range requests."); - } - if (filesize_ == 0) { - ERROR_LOG(LOADER, "Could not determine file size for %s", filename_.c_str()); - } - - // If we didn't end up with a filesize_ (e.g. chunked response), give up. File invalid. + // If we didn't end up with a filesize_ (e.g. chunked response), give up. File invalid. + }); } HTTPFileLoader::~HTTPFileLoader() { @@ -117,6 +114,8 @@ std::string HTTPFileLoader::Path() const { size_t HTTPFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { Prepare(); + std::lock_guard guard(readAtMutex_); + s64 absoluteEnd = std::min(absolutePos + (s64)bytes, filesize_); if (absolutePos >= filesize_ || bytes == 0) { // Read outside of the file or no read at all, just fail immediately. diff --git a/Core/FileLoaders/HTTPFileLoader.h b/Core/FileLoaders/HTTPFileLoader.h index 6a84554da7..5b27bf5288 100644 --- a/Core/FileLoaders/HTTPFileLoader.h +++ b/Core/FileLoaders/HTTPFileLoader.h @@ -17,6 +17,8 @@ #pragma once +#include + #include "net/http_client.h" #include "net/resolve.h" #include "net/url.h" @@ -61,5 +63,7 @@ private: http::Client client_; std::string filename_; bool connected_; - bool prepared_; + + std::once_flag preparedFlag_; + std::mutex readAtMutex_; };