Change the NamedMutex (#1358)

Change the IsCanceled call to the atomic_bool variable.
Upon cancellation the variable is set to true, means we don't lock
the mutex inside CancellationContext. Which eliminates the lock
ordering problem. Move the token reset code out of lock_mutex_.
Switch CI from ubuntu-18.04 to ubuntu-20.04 due to deprecation.
Fix a warning.

Relates-To: OAM-1807

Signed-off-by: Mykhailo Kuchma <ext-mykhailo.kuchma@here.com>

Author: mykhailo-kuchma

.github/workflows/psv_pipelines.yml

@@ -16,7 +16,7 @@ env:
 jobs:
   psv-linux-gcc-build-test-codecov:
     name: PSV / Linux gcc 7.5 / Tests / Code coverage
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-20.04
     steps:
     - name: Check out repository
       uses: actions/checkout@v2
@@ -40,7 +40,7 @@ jobs:
 
   psv-linux-gcc-build-no-cache:
     name: PSV / Linux gcc 7.5 / OLP_SDK_ENABLE_DEFAULT_CACHE=OFF
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-20.04
     env:
       BUILD_TYPE: RelWithDebInfo
     steps:

azure-pipelines.yml

@@ -15,7 +15,7 @@ variables:
 jobs:
 - job: Commit_checker
   pool:
-    vmImage: 'ubuntu-18.04'
+    vmImage: 'ubuntu-20.04'
   condition: eq(variables['Build.Reason'], 'PullRequest')
   steps:
     - bash: scripts/misc/commit_checker.sh

olp-cpp-sdk-authentication/src/AuthenticationClientUtils.cpp

@@ -86,7 +86,7 @@ Response<rapidjson::Document> Parse(client::HttpResponse& http_response) {
                              "Failed to parse response"});
   }
 
-  return std::move(document);
+  return Response<rapidjson::Document>(std::move(document));
 }
 }  // namespace
 

olp-cpp-sdk-dataservice-read/src/repositories/NamedMutex.cpp

@@ -140,21 +140,29 @@ NamedMutex::NamedMutex(NamedMutexStorage& storage, const std::string& name,
       name_{name},
       mutex_{storage_.AcquireLock(name_)},
       lock_condition_{storage_.GetLockCondition(name_)},
-      lock_mutex_{storage_.GetLockMutex(name_)} {}
+      lock_mutex_{storage_.GetLockMutex(name_)},
+      is_canceled_{false} {}
 
 NamedMutex::~NamedMutex() { storage_.ReleaseLock(name_); }
 
 void NamedMutex::lock() {
-  const bool valid = context_.ExecuteOrCancelled(
-      [&]() { return client::CancellationToken([&]() { Notify(); }); });
+  const bool executed = context_.ExecuteOrCancelled([&]() {
+    return client::CancellationToken([&]() {
+      is_canceled_.store(true);
+      Notify();
+    });
+  });
 
-  if (valid) {
+  is_canceled_.store(!executed);
+
+  if (executed) {
     std::unique_lock<std::mutex> unique_lock{lock_mutex_};
     lock_condition_.wait(unique_lock,
-                         [&] { return context_.IsCancelled() || try_lock(); });
-    // Reset the cancel token
-    context_.ExecuteOrCancelled([&]() { return client::CancellationToken(); });
+                         [&] { return is_canceled_ || try_lock(); });
   }
+
+  // Reset the cancel token
+  context_.ExecuteOrCancelled([]() { return client::CancellationToken(); });
 }
 
 bool NamedMutex::try_lock() { return is_locked_ = mutex_.try_lock(); }
@@ -176,6 +184,8 @@ boost::optional<client::ApiError> NamedMutex::GetError() {
 }
 
 void NamedMutex::Notify() {
+  // A mutex is required since there is a gap between predicate call and wait
+  // call when calling wait in NamedMutex::lock.
   std::unique_lock<std::mutex> unique_lock{lock_mutex_};
   lock_condition_.notify_all();
 }

olp-cpp-sdk-dataservice-read/src/repositories/NamedMutex.h

@@ -19,6 +19,7 @@
 
 #pragma once
 
+#include <atomic>
 #include <condition_variable>
 #include <memory>
 #include <mutex>
@@ -121,6 +122,9 @@ class NamedMutex final {
   std::mutex& mutex_;
   std::condition_variable& lock_condition_;
   std::mutex& lock_mutex_;
+  // We can't check CancellationContext::IsCanceled, since it might result in a
+  // deadlock.
+  std::atomic<bool> is_canceled_;
 };
 
 }  // namespace repository