From f7ad2e1e261a5efe50bff3b6f9c1694a74ab92be Mon Sep 17 00:00:00 2001
From: Patrick Niklaus <patrick@mapbox.com>
Date: Tue, 3 Jan 2017 12:06:55 +0100
Subject: [PATCH] Don't retain SharedDataFacade between queries (#3485)

* Don't retain SharedDataFacade between queries

* More caching code
---
 include/engine/data_watchdog.hpp | 55 ++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/include/engine/data_watchdog.hpp b/include/engine/data_watchdog.hpp
index 497602fcc..cf11337ad 100644
--- a/include/engine/data_watchdog.hpp
+++ b/include/engine/data_watchdog.hpp
@@ -22,11 +22,6 @@ namespace engine
 // This class monitors the shared memory region that contains the pointers to
 // the data and layout regions that should be used. This region is updated
 // once a new dataset arrives.
-//
-// TODO: This also needs a shared memory reader lock with other clients and
-// possibly osrm-datastore since updating the CURRENT_REGIONS data is not atomic.
-// Currently we enfore this by waiting that all queries have finished before
-// osrm-datastore writes to this section.
 class DataWatchdog
 {
   public:
@@ -57,7 +52,9 @@ class DataWatchdog
         const auto shared_timestamp =
             static_cast<const storage::SharedDataTimestamp *>(shared_regions->Ptr());
 
-        const auto get_locked_facade = [this, shared_timestamp]() {
+
+        const auto get_locked_facade = [this, shared_timestamp](
+            const std::shared_ptr<datafacade::SharedMemoryDataFacade> &facade) {
             if (current_timestamp.region == storage::REGION_1)
             {
                 return std::make_pair(RegionsLock(shared_barriers->region_1_mutex), facade);
@@ -76,8 +73,11 @@ class DataWatchdog
 
             if (shared_timestamp->timestamp == current_timestamp.timestamp)
             {
-                BOOST_ASSERT(shared_timestamp->region == current_timestamp.region);
-                return get_locked_facade();
+                if (auto facade = cached_facade.lock())
+                {
+                    BOOST_ASSERT(shared_timestamp->region == current_timestamp.region);
+                    return get_locked_facade(facade);
+                }
             }
         }
 
@@ -89,20 +89,41 @@ class DataWatchdog
         // in that case we don't modify anything
         if (shared_timestamp->timestamp == current_timestamp.timestamp)
         {
-            BOOST_ASSERT(shared_timestamp->region == current_timestamp.region);
-
-            return get_locked_facade();
+            if (auto facade = cached_facade.lock())
+            {
+                BOOST_ASSERT(shared_timestamp->region == current_timestamp.region);
+                return get_locked_facade(facade);
+            }
         }
 
         // this thread has won and can update the data
         boost::upgrade_to_unique_lock<boost::upgrade_mutex> unique_facade_lock(facade_lock);
 
-        current_timestamp = *shared_timestamp;
-        facade = std::make_shared<datafacade::SharedMemoryDataFacade>(shared_barriers,
-                                                                      current_timestamp.region,
-                                                                      current_timestamp.timestamp);
+        // if two threads try to enter this critical section one will loose
+        // and will find an up-to-date instance of the shared data facade
+        if (shared_timestamp->timestamp == current_timestamp.timestamp)
+        {
+            // if the thread that updated the facade finishes the query before
+            // we can aquire our handle here, we need to regenerate
+            if (auto facade = cached_facade.lock())
+            {
+                BOOST_ASSERT(shared_timestamp->region == current_timestamp.region);
 
-        return get_locked_facade();
+                return get_locked_facade(facade);
+            }
+        }
+        else
+        {
+            current_timestamp = *shared_timestamp;
+        }
+
+        auto new_facade =
+            std::make_shared<datafacade::SharedMemoryDataFacade>(shared_barriers,
+                                                                 current_timestamp.region,
+                                                                 current_timestamp.timestamp);
+        cached_facade = new_facade;
+
+        return get_locked_facade(new_facade);
     }
 
   private:
@@ -114,7 +135,7 @@ class DataWatchdog
     std::unique_ptr<storage::SharedMemory> shared_regions;
 
     mutable boost::shared_mutex facade_mutex;
-    std::shared_ptr<datafacade::SharedMemoryDataFacade> facade;
+    std::weak_ptr<datafacade::SharedMemoryDataFacade> cached_facade;
     storage::SharedDataTimestamp current_timestamp;
 };
 }