From f5b521ffb51e396be322350a204b835a0bf0af08 Mon Sep 17 00:00:00 2001
From: "Thiago Marcos P. Santos" <thiago@mapbox.com>
Date: Tue, 16 Feb 2016 15:51:40 +0200
Subject: [core] Remove requests from the pending queue on cancel

If we don't do this, we can activate a request twice.

The issue here is that we use memory address as the 'key'. So if
we destroy a request, another request might take the same memory
address, or the same 'key'.

The entry at 'pendingRequests' survives the 'cancel()' and is
ultimatelly removed at 'activatePendingRequest()'. This is fine
unless a request is created using the same 'key' before we call
'activatePendingRequest()'.

The check if the request was canceled at 'activatePendingRequest()'
will be no longer valid, activating the request. At the same time
the request will be activated again by the task scheduled at the
constructor of the 'OnlineFileRequestImpl' class.
---
 platform/default/online_file_source.cpp | 34 ++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

(limited to 'platform/default/online_file_source.cpp')

diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp
index a095b86fba..43c707775d 100644
--- a/platform/default/online_file_source.cpp
+++ b/platform/default/online_file_source.cpp
@@ -15,6 +15,7 @@
 
 #include <algorithm>
 #include <cassert>
+#include <list>
 #include <set>
 #include <unordered_map>
 
@@ -62,6 +63,12 @@ public:
         allRequests.erase(key);
         if (activeRequests.erase(key)) {
             activatePendingRequest();
+        } else {
+            auto it = pendingRequestsMap.find(key);
+            if (it != pendingRequestsMap.end()) {
+                pendingRequestsList.erase(it->second);
+                pendingRequestsMap.erase(it);
+            }
         }
     }
 
@@ -78,7 +85,8 @@ public:
     }
 
     void queueRequest(OnlineFileRequestImpl* impl) {
-        pendingRequests.push(impl->key);
+        auto it = pendingRequestsList.insert(pendingRequestsList.end(), impl->key);
+        pendingRequestsMap.emplace(impl->key, std::move(it));
     }
 
     void activateRequest(OnlineFileRequestImpl* impl) {
@@ -92,19 +100,18 @@ public:
     }
 
     void activatePendingRequest() {
-        while (!pendingRequests.empty()) {
-            FileRequest* key = pendingRequests.front();
-            pendingRequests.pop();
-
-            auto it = allRequests.find(key);
-            if (it == allRequests.end()) {
-                // It must have been cancelled.
-                continue;
-            }
-
-            activateRequest(it->second.get());
+        if (pendingRequestsList.empty()) {
             return;
         }
+
+        FileRequest* key = pendingRequestsList.front();
+        pendingRequestsList.pop_front();
+
+        pendingRequestsMap.erase(key);
+
+        auto it = allRequests.find(key);
+        assert(it != allRequests.end());
+        activateRequest(it->second.get());
     }
 
 private:
@@ -126,7 +133,8 @@ private:
      * `pendingRequests`. Requests in the active state are in `activeRequests`.
      */
     std::unordered_map<FileRequest*, std::unique_ptr<OnlineFileRequestImpl>> allRequests;
-    std::queue<FileRequest*> pendingRequests;
+    std::list<FileRequest*> pendingRequestsList;
+    std::unordered_map<FileRequest*, std::list<FileRequest*>::iterator> pendingRequestsMap;
     std::set<FileRequest*> activeRequests;
 
     const std::unique_ptr<HTTPContextBase> httpContext { HTTPContextBase::createContext() };
-- 
cgit v1.2.1