diff options
author | John Firebaugh <john.firebaugh@gmail.com> | 2016-03-24 11:33:09 -0700 |
---|---|---|
committer | John Firebaugh <john.firebaugh@gmail.com> | 2016-04-14 14:18:19 -0700 |
commit | 6d88a23a60bc0a6dc9945bf09a659d15dd827192 (patch) | |
tree | 502272909f636b2eef8b40636ce6330211753da1 | |
parent | 7bb74ac8db190c8e73c013879b95dc85ea38c2ae (diff) | |
download | qtlocation-mapboxgl-6d88a23a60bc0a6dc9945bf09a659d15dd827192.tar.gz |
[core] Remove internal threading from OnlineFileRequest
There's no need to do the work that OnlineFileRequest does on a separate thread from the DefaultFileSource thread, and having AsyncTasks proxy to other tasks across a thread boundary adds needless complexity.
-rw-r--r-- | include/mbgl/storage/online_file_source.hpp | 8 | ||||
-rw-r--r-- | platform/default/online_file_source.cpp | 129 | ||||
-rw-r--r-- | test/map/map_context.cpp | 3 |
3 files changed, 64 insertions, 76 deletions
diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp index b52dce35f0..fd742a54c4 100644 --- a/include/mbgl/storage/online_file_source.hpp +++ b/include/mbgl/storage/online_file_source.hpp @@ -5,10 +5,6 @@ namespace mbgl { -namespace util { -template <typename T> class Thread; -} // namespace util - class OnlineFileSource : public FileSource { public: OnlineFileSource(); @@ -20,10 +16,10 @@ public: std::unique_ptr<AsyncRequest> request(const Resource&, Callback) override; private: - friend class OnlineFileRequestImpl; + friend class OnlineFileRequest; class Impl; - const std::unique_ptr<util::Thread<Impl>> thread; + const std::unique_ptr<Impl> impl; std::string accessToken; }; diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp index dcd14add8b..533046850a 100644 --- a/platform/default/online_file_source.cpp +++ b/platform/default/online_file_source.cpp @@ -22,18 +22,18 @@ namespace mbgl { -class OnlineFileRequestImpl : public util::noncopyable { +class OnlineFileRequest : public AsyncRequest { public: using Callback = std::function<void (Response)>; - OnlineFileRequestImpl(AsyncRequest*, const Resource&, Callback, OnlineFileSource::Impl&); - ~OnlineFileRequestImpl(); + OnlineFileRequest(const Resource&, Callback, OnlineFileSource::Impl&); + ~OnlineFileRequest(); - void networkIsReachableAgain(OnlineFileSource::Impl&); - void schedule(OnlineFileSource::Impl&, optional<SystemTimePoint> expires); - void completed(OnlineFileSource::Impl&, Response); + void networkIsReachableAgain(); + void schedule(optional<SystemTimePoint> expires); + void completed(Response); - AsyncRequest* key; + OnlineFileSource::Impl& impl; Resource resource; HTTPRequestBase* request = nullptr; util::Timer timer; @@ -52,8 +52,7 @@ public: class OnlineFileSource::Impl { public: - // Dummy parameter is a workaround for a gcc 4.9 bug. - Impl(int) { + Impl() { NetworkStatus::Subscribe(&reachability); } @@ -61,16 +60,16 @@ public: NetworkStatus::Unsubscribe(&reachability); } - void request(AsyncRequest* key, Resource resource, Callback callback) { - allRequests[key] = std::make_unique<OnlineFileRequestImpl>(key, resource, callback, *this); + void add(OnlineFileRequest* request) { + allRequests.insert(request); } - void cancel(AsyncRequest* key) { - allRequests.erase(key); - if (activeRequests.erase(key)) { + void remove(OnlineFileRequest* request) { + allRequests.erase(request); + if (activeRequests.erase(request)) { activatePendingRequest(); } else { - auto it = pendingRequestsMap.find(key); + auto it = pendingRequestsMap.find(request); if (it != pendingRequestsMap.end()) { pendingRequestsList.erase(it->second); pendingRequestsMap.erase(it); @@ -78,30 +77,30 @@ public: } } - void activateOrQueueRequest(OnlineFileRequestImpl* impl) { - assert(allRequests.find(impl->key) != allRequests.end()); - assert(activeRequests.find(impl->key) == activeRequests.end()); - assert(!impl->request); + void activateOrQueueRequest(OnlineFileRequest* request) { + assert(allRequests.find(request) != allRequests.end()); + assert(activeRequests.find(request) == activeRequests.end()); + assert(!request->request); if (activeRequests.size() >= HTTPContextBase::maximumConcurrentRequests()) { - queueRequest(impl); + queueRequest(request); } else { - activateRequest(impl); + activateRequest(request); } } - void queueRequest(OnlineFileRequestImpl* impl) { - auto it = pendingRequestsList.insert(pendingRequestsList.end(), impl->key); - pendingRequestsMap.emplace(impl->key, std::move(it)); + void queueRequest(OnlineFileRequest* request) { + auto it = pendingRequestsList.insert(pendingRequestsList.end(), request); + pendingRequestsMap.emplace(request, std::move(it)); } - void activateRequest(OnlineFileRequestImpl* impl) { - activeRequests.insert(impl->key); - impl->request = httpContext->createRequest(impl->resource, [=] (Response response) { - impl->request = nullptr; - activeRequests.erase(impl->key); + void activateRequest(OnlineFileRequest* request) { + activeRequests.insert(request); + request->request = httpContext->createRequest(request->resource, [=] (Response response) { + activeRequests.erase(request); activatePendingRequest(); - impl->completed(*this, response); + request->request = nullptr; + request->completed(response); }); } @@ -110,20 +109,18 @@ public: return; } - AsyncRequest* key = pendingRequestsList.front(); + OnlineFileRequest* request = pendingRequestsList.front(); pendingRequestsList.pop_front(); - pendingRequestsMap.erase(key); + pendingRequestsMap.erase(request); - auto it = allRequests.find(key); - assert(it != allRequests.end()); - activateRequest(it->second.get()); + activateRequest(request); } private: void networkIsReachableAgain() { - for (auto& req : allRequests) { - req.second->networkIsReachableAgain(*this); + for (auto& request : allRequests) { + request->networkIsReachableAgain(); } } @@ -138,18 +135,17 @@ private: * Requests in any state are in `allRequests`. Requests in the pending state are in * `pendingRequests`. Requests in the active state are in `activeRequests`. */ - std::unordered_map<AsyncRequest*, std::unique_ptr<OnlineFileRequestImpl>> allRequests; - std::list<AsyncRequest*> pendingRequestsList; - std::unordered_map<AsyncRequest*, std::list<AsyncRequest*>::iterator> pendingRequestsMap; - std::unordered_set<AsyncRequest*> activeRequests; + std::unordered_set<OnlineFileRequest*> allRequests; + std::list<OnlineFileRequest*> pendingRequestsList; + std::unordered_map<OnlineFileRequest*, std::list<OnlineFileRequest*>::iterator> pendingRequestsMap; + std::unordered_set<OnlineFileRequest*> activeRequests; const std::unique_ptr<HTTPContextBase> httpContext { HTTPContextBase::createContext() }; util::AsyncTask reachability { std::bind(&Impl::networkIsReachableAgain, this) }; }; OnlineFileSource::OnlineFileSource() - : thread(std::make_unique<util::Thread<Impl>>( - util::ThreadContext{ "OnlineFileSource", util::ThreadType::Unknown, util::ThreadPriority::Low }, 0)) { + : impl(std::make_unique<Impl>()) { } OnlineFileSource::~OnlineFileSource() = default; @@ -183,37 +179,25 @@ std::unique_ptr<AsyncRequest> OnlineFileSource::request(const Resource& resource break; } - class OnlineFileRequest : public AsyncRequest { - public: - OnlineFileRequest(Resource resource_, FileSource::Callback callback_, util::Thread<OnlineFileSource::Impl>& thread_) - : thread(thread_), - workRequest(thread.invokeWithCallback(&OnlineFileSource::Impl::request, callback_, this, resource_)) { - } - - ~OnlineFileRequest() override { - thread.invoke(&OnlineFileSource::Impl::cancel, this); - } - - util::Thread<OnlineFileSource::Impl>& thread; - std::unique_ptr<AsyncRequest> workRequest; - }; - - return std::make_unique<OnlineFileRequest>(res, callback, *thread); + return std::make_unique<OnlineFileRequest>(res, callback, *impl); } -OnlineFileRequestImpl::OnlineFileRequestImpl(AsyncRequest* key_, const Resource& resource_, Callback callback_, OnlineFileSource::Impl& impl) - : key(key_), +OnlineFileRequest::OnlineFileRequest(const Resource& resource_, Callback callback_, OnlineFileSource::Impl& impl_) + : impl(impl_), resource(resource_), callback(std::move(callback_)) { + impl.add(this); + // Force an immediate first request if we don't have an expiration time. if (resource.priorExpires) { - schedule(impl, resource.priorExpires); + schedule(resource.priorExpires); } else { - schedule(impl, SystemClock::now()); + schedule(SystemClock::now()); } } -OnlineFileRequestImpl::~OnlineFileRequestImpl() { +OnlineFileRequest::~OnlineFileRequest() { + impl.remove(this); if (request) { request->cancel(); } @@ -278,7 +262,7 @@ SystemTimePoint interpolateExpiration(const SystemTimePoint& current, return now + std::max<SystemDuration>(delta, util::CLOCK_SKEW_RETRY_TIMEOUT); } -void OnlineFileRequestImpl::schedule(OnlineFileSource::Impl& impl, optional<SystemTimePoint> expires) { +void OnlineFileRequest::schedule(optional<SystemTimePoint> expires) { if (request) { // There's already a request in progress; don't start another one. return; @@ -307,7 +291,7 @@ void OnlineFileRequestImpl::schedule(OnlineFileSource::Impl& impl, optional<Syst }); } -void OnlineFileRequestImpl::completed(OnlineFileSource::Impl& impl, Response response) { +void OnlineFileRequest::completed(Response response) { // If we didn't get various caching headers in the response, continue using the // previous values. Otherwise, update the previous values to the new values. @@ -345,15 +329,20 @@ void OnlineFileRequestImpl::completed(OnlineFileSource::Impl& impl, Response res failedRequestReason = Response::Error::Reason::Success; } - callback(response); - schedule(impl, response.expires); + schedule(response.expires); + + // Calling the callback may result in `this` being deleted. It needs to be done last, + // and needs to make a local copy of the callback to ensure that it remains valid for + // the duration of the call. + auto callback_ = callback; + callback_(response); } -void OnlineFileRequestImpl::networkIsReachableAgain(OnlineFileSource::Impl& impl) { +void OnlineFileRequest::networkIsReachableAgain() { // We need all requests to fail at least once before we are going to start retrying // them, and we only immediately restart request that failed due to connection issues. if (failedRequestReason == Response::Error::Reason::Connection) { - schedule(impl, SystemClock::now()); + schedule(SystemClock::now()); } } diff --git a/test/map/map_context.cpp b/test/map/map_context.cpp index a1152d68e7..5de0490557 100644 --- a/test/map/map_context.cpp +++ b/test/map/map_context.cpp @@ -6,10 +6,13 @@ #include <mbgl/platform/default/headless_display.hpp> #include <mbgl/storage/online_file_source.hpp> #include <mbgl/util/thread.hpp> +#include <mbgl/util/run_loop.hpp> using namespace mbgl; TEST(MapContext, DoubleStyleLoad) { + util::RunLoop loop; + std::shared_ptr<HeadlessDisplay> display = std::make_shared<HeadlessDisplay>(); HeadlessView view(display, 1, 512, 512); OnlineFileSource fileSource; |