Fixed test CancelPendingRequestsCatalog

Made PendingRequests::CancelPendingRequests() to first cancel all requests then check for completion
Now ErrorCode::RequestTimeout is propagated properly to user

Resolves: OLPEDGE-678

Signed-off-by: Serhii Lozynskyi <[email protected]>

Author: meravingen123

olp-cpp-sdk-core/include/olp/core/client/PendingRequests.h

@@ -40,10 +40,18 @@ class PendingRequests final {
   ~PendingRequests();
 
   /**
-   * @brief Cancels all pending requests.
+   * @brief Cancels all pending tasks
+   * @note This call does not wait for the tasks to finalize, use
+   * CancelAllAndWait() to also wait for the tasks to finalize..
    * @return True on success
    */
-  bool CancelPendingRequests();
+  bool CancelAll();
+  
+  /**
+   * @brief Cancels all pending tasks and waits for all beeing finalized.
+   * @return True on success
+   */
+  bool CancelAllAndWait();
 
   /**
    * @brief Generates a placehoder for request cancellation token and returns a
@@ -82,11 +90,15 @@ class PendingRequests final {
 
  private:
   int64_t key_ = 0;
-  std::unordered_map<int64_t, client::CancellationToken> requests_map_;
-  std::unordered_set<client::TaskContext, client::TaskContextHash>
-      task_contexts_;
+  using RequestMap = std::unordered_map<int64_t, client::CancellationToken>;
+  using ContextMap =
+      std::unordered_set<client::TaskContext, client::TaskContextHash>;
+  RequestMap requests_map_;
+  ContextMap task_contexts_;
+
   std::mutex requests_lock_;
 };
 
 }  // namespace client
-}  // namespace olp
+}  // namespace olp
+

olp-cpp-sdk-core/include/olp/core/client/TaskContext.h

@@ -150,7 +150,9 @@ class TaskContext {
         auto response = function(context_);
         // Cancel could occur during function execution, in that case we ignore
         // the response.
-        if (!context_.IsCancelled()) {
+        if (!context_.IsCancelled() ||
+            (!response.IsSuccessful() &&
+             response.GetError().GetErrorCode() == ErrorCode::RequestTimeout)) {
           user_response = std::move(response);
         }
       }
@@ -170,7 +172,9 @@ class TaskContext {
       }
 
       // Cancel operation and wait for notification
-      context_.CancelOperation();
+      if (!context_.IsCancelled()) {
+        context_.CancelOperation();
+      }
 
       {
         std::lock_guard<std::mutex> lock(mutex_);

olp-cpp-sdk-core/src/client/PendingRequests.cpp

@@ -18,7 +18,6 @@
  */
 
 #include <olp/core/client/PendingRequests.h>
-
 #include <olp/core/client/TaskContext.h>
 #include <olp/core/logging/Log.h>
 
@@ -33,17 +32,35 @@ PendingRequests::PendingRequests(){};
 
 PendingRequests::~PendingRequests(){};
 
-bool PendingRequests::CancelPendingRequests() {
-  requests_lock_.lock();
-  // Create local copy of the requests to cancel
-  auto contexts = std::move(task_contexts_);
-  auto requests_map = requests_map_;
+bool PendingRequests::CancelAll() {
+  RequestMap requests_map;
+  ContextMap contexts;
+  {
+    std::lock_guard<std::mutex> lk(requests_lock_);
+    requests_map = requests_map_;
+    contexts = task_contexts_;
+  }
 
-  requests_lock_.unlock();
   for (auto& pair : requests_map) {
     pair.second.cancel();
   }
 
+  for (auto context : contexts) {
+    context.CancelToken().cancel();
+  }
+
+  return true;
+}
+
+bool PendingRequests::CancelAllAndWait() {
+  CancelAll();
+
+  ContextMap contexts;
+  {
+    std::lock_guard<std::mutex> lk(requests_lock_);
+    contexts = std::move(task_contexts_);
+  }
+
   for (auto context : contexts) {
     if (!context.BlockingCancel()) {
       OLP_SDK_LOG_WARNING(kLogTag, "Timeout, when waiting on BlockingCancel");

olp-cpp-sdk-core/tests/client/PendingRequestsTest.cpp

@@ -36,13 +36,13 @@ TEST(PendingRequestsTest, RemoveMissingKeyWillFail) {
   EXPECT_FALSE(pending_request.Remove(0));
 }
 
-TEST(PendingRequestsTest, CancelAllPendingRequest) {
+TEST(PendingRequestsTest, CancelAll) {
   PendingRequests pending_request;
   auto key = pending_request.GenerateRequestPlaceholder();
   bool cancelled = false;
   auto token = olp::client::CancellationToken([&]() { cancelled = true; });
   EXPECT_TRUE(pending_request.Insert(token, key));
-  EXPECT_TRUE(pending_request.CancelPendingRequests());
+  EXPECT_TRUE(pending_request.CancelAll());
   EXPECT_TRUE(cancelled);
 }
 

olp-cpp-sdk-dataservice-read/src/CatalogClientImpl.cpp

@@ -23,6 +23,7 @@
 #include <olp/core/client/OlpClientSettingsFactory.h>
 #include <olp/core/client/PendingRequests.h>
 #include <olp/core/logging/Log.h>
+
 #include "Common.h"
 #include "PrefetchTilesProvider.h"
 #include "repositories/ApiRepository.h"
@@ -55,11 +56,13 @@ CatalogClientImpl::CatalogClientImpl(HRN catalog, OlpClientSettings settings)
   pending_requests_ = std::make_shared<client::PendingRequests>();
 }
 
-CatalogClientImpl::~CatalogClientImpl() { CancelPendingRequests(); }
+CatalogClientImpl::~CatalogClientImpl() {
+  pending_requests_->CancelAllAndWait();
+}
 
 bool CatalogClientImpl::CancelPendingRequests() {
   OLP_SDK_LOG_TRACE(kLogTag, "CancelPendingRequests");
-  return pending_requests_->CancelPendingRequests();
+  return pending_requests_->CancelAll();
 }
 
 CancellationToken CatalogClientImpl::GetCatalog(

olp-cpp-sdk-dataservice-read/src/VersionedLayerClientImpl.cpp

@@ -77,7 +77,7 @@ VersionedLayerClientImpl::VersionedLayerClientImpl(
 }
 
 VersionedLayerClientImpl::~VersionedLayerClientImpl() {
-  pending_requests_->CancelPendingRequests();
+  pending_requests_->CancelAllAndWait();
 }
 
 client::CancellationToken VersionedLayerClientImpl::GetPartitions(

olp-cpp-sdk-dataservice-read/src/VolatileLayerClientImpl.cpp

@@ -68,7 +68,7 @@ VolatileLayerClientImpl::VolatileLayerClientImpl(
 }
 
 VolatileLayerClientImpl::~VolatileLayerClientImpl() {
-  pending_requests_->CancelPendingRequests();
+  pending_requests_->CancelAllAndWait();
 }
 
 client::CancellationToken VolatileLayerClientImpl::GetPartitions(

tests/integration/olp-cpp-sdk-dataservice-read/CatalogClientTest.cpp

@@ -25,6 +25,8 @@
 #include <olp/core/logging/Log.h>
 #include <olp/core/porting/make_unique.h>
 #include <olp/dataservice/read/CatalogClient.h>
+#include <olp/dataservice/read/model/Catalog.h>
+
 #include "CatalogClientTestBase.h"
 #include "HttpResponses.h"
 
@@ -416,30 +418,26 @@ TEST_P(CatalogClientTest, GetCatalog403CacheClear) {
   ASSERT_FALSE(catalog_response.IsSuccessful());
 }
 
-TEST_P(CatalogClientTest, DISABLED_CancelPendingRequestsCatalog) {
+TEST_P(CatalogClientTest, CancelPendingRequestsCatalog) {
   olp::client::HRN hrn(GetTestCatalog());
   testing::InSequence s;
-  std::vector<std::shared_ptr<std::promise<void>>> waits;
   std::vector<std::shared_ptr<std::promise<void>>> pauses;
 
   auto catalog_client = std::make_unique<CatalogClient>(hrn, settings_);
   auto catalog_request = CatalogRequest().WithFetchOption(OnlineOnly);
   auto version_request = CatalogVersionRequest().WithFetchOption(OnlineOnly);
 
   // Make a few requests
-  auto wait_for_cancel1 = std::make_shared<std::promise<void>>();
-  auto pause_for_cancel1 = std::make_shared<std::promise<void>>();
-  auto wait_for_cancel2 = std::make_shared<std::promise<void>>();
-  auto pause_for_cancel2 = std::make_shared<std::promise<void>>();
+  auto wait_for_cancel = std::make_shared<std::promise<void>>();
+  auto pause_for_cancel = std::make_shared<std::promise<void>>();
 
   {
     olp::http::RequestId request_id;
     NetworkCallback send_mock;
     CancelCallback cancel_mock;
 
-    std::tie(request_id, send_mock, cancel_mock) =
-        GenerateNetworkMockActions(wait_for_cancel1, pause_for_cancel1,
-                                   {200, HTTP_RESPONSE_LOOKUP_CONFIG});
+    std::tie(request_id, send_mock, cancel_mock) = GenerateNetworkMockActions(
+        wait_for_cancel, pause_for_cancel, {200, HTTP_RESPONSE_LOOKUP_CONFIG});
 
     EXPECT_CALL(*network_mock_,
                 Send(IsGetRequest(URL_LOOKUP_CONFIG), _, _, _, _))
@@ -450,44 +448,19 @@ TEST_P(CatalogClientTest, DISABLED_CancelPendingRequestsCatalog) {
         .WillOnce(testing::Invoke(std::move(cancel_mock)));
   }
 
-  {
-    olp::http::RequestId request_id;
-    NetworkCallback send_mock;
-    CancelCallback cancel_mock;
-
-    std::tie(request_id, send_mock, cancel_mock) =
-        GenerateNetworkMockActions(wait_for_cancel2, pause_for_cancel2,
-                                   {200, HTTP_RESPONSE_LOOKUP_METADATA});
-
-    EXPECT_CALL(*network_mock_,
-                Send(IsGetRequest(URL_LOOKUP_METADATA), _, _, _, _))
-        .Times(1)
-        .WillOnce(testing::Invoke(std::move(send_mock)));
-
-    EXPECT_CALL(*network_mock_, Cancel(request_id))
-        .WillOnce(testing::Invoke(std::move(cancel_mock)));
-  }
-
-  waits.push_back(wait_for_cancel1);
-  pauses.push_back(pause_for_cancel1);
   auto catalog_future = catalog_client->GetCatalog(catalog_request);
-
-  waits.push_back(wait_for_cancel2);
-  pauses.push_back(pause_for_cancel2);
   auto version_future = catalog_client->GetLatestVersion(version_request);
 
-  for (auto wait : waits) {
-    wait->get_future().get();
-  }
+  // We are using only one thread so we can only have one network request
+  // active. So just wait for it.
+  wait_for_cancel->get_future().get();
+
   // Cancel them all
   catalog_client->CancelPendingRequests();
-  for (auto pause : pauses) {
-    pause->set_value();
-  }
+  pause_for_cancel->set_value();
 
   // Verify they are all cancelled
   CatalogResponse catalog_response = catalog_future.GetFuture().get();
-
   ASSERT_FALSE(catalog_response.IsSuccessful())
       << ApiErrorToString(catalog_response.GetError());