[core] do not use std::atomic_* with shared_ptrs

It's not implemented in GCC 4.9.2's stdlib (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57250). Instead, we're now always using a mutex to protect access; we previously created a mutex only on cancelation, but since we're always canceling now, it makes sense to allocate it right away.

1 files changed, 25 insertions, 26 deletions

diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp
index 55913846cf..5ed9278d0a 100644
--- a/src/mbgl/storage/request.cpp
+++ b/src/mbgl/storage/request.cpp
@@ -6,12 +6,10 @@
 
 #include <cassert>
 #include <functional>
-#include <atomic>
+#include <memory>
 
 namespace mbgl {
 
-struct Request::Canceled { std::mutex mutex; bool confirmed = false; };
-
 // Note: This requires that loop is running in the current thread (or not yet running).
 Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_)
     : async(std::make_unique<uv::async>(loop, [this] { notifyCallback(); })),
@@ -21,56 +19,57 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_)
 
 // Called in the originating thread.
 void Request::notifyCallback() {
+    std::unique_lock<std::mutex> lock(mtx);
     if (!canceled) {
-        invoke();
-    } else {
-        bool destroy = false;
-        {
-            std::unique_lock<std::mutex> lock(canceled->mutex);
-            destroy = canceled->confirmed;
+        // Move the response object out so that we can't accidentally notify twice.
+        auto res = std::move(response);
+        assert(!response);
+        // Unlock before, since callbacks may call cancel, which also locks this mutex.
+        lock.unlock();
+        // The user could supply a null pointer or empty std::function as a callback. In this case, we
+        // still do the file request, but we don't need to deliver a result.
+        // Similarly, two consecutive updates could trigger two notifyCallbacks, so we need to make
+        // sure
+        if (callback && res) {
+            callback(*res);
         }
+    } else {
         // Don't delete right way, because we have to unlock the mutex before deleting.
-        if (destroy) {
+        if (confirmed) {
+            lock.unlock();
             delete this;
         }
     }
 }
 
-void Request::invoke() {
-    assert(response);
-    // The user could supply a null pointer or empty std::function as a callback. In this case, we
-    // still do the file request, but we don't need to deliver a result.
-    if (callback) {
-        callback(*std::atomic_load(&response));
-    }
-}
-
 Request::~Request() = default;
 
 // Called in the FileSource thread.
 void Request::notify(const std::shared_ptr<const Response> &response_) {
+    std::lock_guard<std::mutex> lock(mtx);
     assert(response_);
-    std::atomic_store(&response, response_);
+    response = response_;
     async->send();
 }
 
 // Called in the originating thread.
 void Request::cancel() {
-    assert(async);
+    std::lock_guard<std::mutex> lock(mtx);
     assert(!canceled);
-    canceled = std::make_unique<Canceled>();
+    canceled = true;
 }
 
 
 // Called in the FileSource thread.
 // Will only ever be invoked after cancel() was called in the original requesting thread.
 void Request::destruct() {
-    assert(async);
+    std::lock_guard<std::mutex> lock(mtx);
     assert(canceled);
-    std::unique_lock<std::mutex> lock(canceled->mutex);
-    canceled->confirmed = true;
+    confirmed = true;
+    // We need to extend the lock until after the async has been sent, otherwise the requesting
+    // thread could destroy the async while this call is still in progress.
     async->send();
-    // after this method returns, the FileSource thread has no knowledge of
+    // After this method returns, the FileSource thread has no knowledge of
     // this object anymore.
 }