[native, managed] Bug fix in how default timeout is used (#17)

Inc/IPC/Managed/detail/Interop/ClientConnectorImpl.h

@@ -25,19 +25,13 @@ namespace Managed
         template <typename Request, typename Response>
         void Transport<Request, Response>::ClientConnector::Connect(const char* acceptorName, HandlerFactory&& handlerFactory, const std::chrono::milliseconds& timeout)
         {
-            auto handler = [handlerFactory = std::move(handlerFactory)](auto&& futureConnection) mutable
-            {
-                handlerFactory([futureConnection = std::move(futureConnection)]() mutable { return futureConnection.get(); });
-            };
-
-            if (timeout == std::chrono::milliseconds::zero())
-            {
-                this->get()->Connect(acceptorName, std::move(handler));
-            }
-            else
-            {
-                this->get()->Connect(acceptorName, std::move(handler), timeout);
-            }
+            this->get()->Connect(
+                acceptorName,
+                [handlerFactory = std::move(handlerFactory)](auto&& futureConnection) mutable
+                {
+                    handlerFactory([futureConnection = std::move(futureConnection)]() mutable { return futureConnection.get(); });
+                },
+                timeout);
         }
 
     } // Interop

Inc/IPC/Managed/detail/Interop/ClientImpl.h

@@ -46,14 +46,7 @@ namespace Managed
         template <typename Request, typename Response>
         void Transport<Request, Response>::Client::operator()(const Request& request, Callback<void(Response&&)>&& callback, const std::chrono::milliseconds& timeout)
         {
-            if (timeout == std::chrono::milliseconds::zero())
-            {
-                this->get()->operator()(request, std::move(callback));
-            }
-            else
-            {
-                this->get()->operator()(request, std::move(callback), timeout);
-            }
+            this->get()->operator()(request, std::move(callback), timeout);
         }
 
         template <typename Request, typename Response>

Inc/IPC/Policies/TransactionManager.h

@@ -4,6 +4,7 @@
 #include "NullTimeoutFactory.h"
 #include "IPC/detail/LockFree/IndexedObjectPool.h"
 #include <chrono>
+#include <cassert>
 
 #pragma warning(push)
 #include <boost/optional.hpp>
@@ -24,17 +25,11 @@ namespace Policies
 
         explicit TransactionManager(TimeoutFactory timeoutFactory, const std::chrono::milliseconds& defaultTimeout = {})
             : m_timeoutFactory{ std::move(timeoutFactory) },
-              m_defaultTimeout{ defaultTimeout }
+              m_defaultTimeout{ NonZeroTimeout(defaultTimeout) }
         {}
 
         template <typename OtherContext>
-        Id BeginTransaction(OtherContext&& context)
-        {
-            return BeginTransaction(std::forward<OtherContext>(context), m_defaultTimeout);
-        }
-
-        template <typename OtherContext>
-        Id BeginTransaction(OtherContext&& context, const std::chrono::milliseconds& timeout)
+        Id BeginTransaction(OtherContext&& context, const std::chrono::milliseconds& timeout = {})
         {
             auto result = m_transactions->Take(
                 [this](Id id)
@@ -47,9 +42,7 @@ namespace Policies
 
             try
             {
-                transaction.Begin(
-                    std::forward<OtherContext>(context),
-                    timeout != std::chrono::milliseconds::zero() ? timeout : GetDefaultTimeout());
+                transaction.Begin(std::forward<OtherContext>(context), NonZeroTimeout(timeout, m_defaultTimeout));
             }
             catch (...)
             {
@@ -74,6 +67,11 @@ namespace Policies
             m_transactions->ReturnAll([](auto& transaction) { transaction.End(); });
         }
 
+        const std::chrono::milliseconds& GetDefaultTimeout() const
+        {
+            return m_defaultTimeout;
+        }
+
     private:
         class Transaction
         {
@@ -86,6 +84,7 @@ namespace Policies
             template <typename OtherContext>
             void Begin(OtherContext&& context, const std::chrono::milliseconds& timeout)
             {
+                assert(timeout != std::chrono::milliseconds::zero());
                 m_context = std::forward<OtherContext>(context);
                 m_timeoutScheduler(timeout);
             }
@@ -110,15 +109,21 @@ namespace Policies
 
         static_assert(std::is_same<Id, typename TransactionPool::Index>::value, "Id and Index must have the same type.");
 
-        static constexpr auto GetDefaultTimeout()
+        static constexpr auto NonZeroTimeout()
         {
             return std::chrono::seconds{ 3 };
         }
 
+        template <typename Rep, typename Period, typename... Timeouts>
+        static constexpr decltype(auto) NonZeroTimeout(const std::chrono::duration<Rep, Period>& timeout, Timeouts&&... timeouts)
+        {
+            return timeout != std::chrono::duration<Rep, Period>::zero() ? timeout : NonZeroTimeout(std::forward<Timeouts>(timeouts)...);
+        }
+
 
         std::unique_ptr<TransactionPool> m_transactions{ std::make_unique<TransactionPool>() };
         TimeoutFactory m_timeoutFactory;
-        std::chrono::milliseconds m_defaultTimeout{};
+        std::chrono::milliseconds m_defaultTimeout{ NonZeroTimeout() };
     };
 
 } // Policies

UnitTests/TransactionManagerTests.cpp

@@ -128,4 +128,34 @@ BOOST_AUTO_TEST_CASE(TimeoutTest)
     BOOST_TEST(!transactions.EndTransaction(id2));
 }
 
+BOOST_AUTO_TEST_CASE(DefaultTimeoutTest)
+{
+    UnitTest::Mocks::TimeoutFactory timeouts;
+    Policies::TransactionManager<int, decltype(timeouts)> transactions{ timeouts };
+
+    auto check = [&]
+    {
+        transactions.BeginTransaction(1, std::chrono::milliseconds{ 100 });
+        BOOST_TEST(timeouts->size() == 1);
+        BOOST_TEST((timeouts->back().second == std::chrono::milliseconds{ 100 }));
+
+        transactions.BeginTransaction(2);
+        BOOST_TEST(timeouts->size() == 2);
+        BOOST_TEST((timeouts->back().second == transactions.GetDefaultTimeout()));
+
+        transactions.BeginTransaction(3, std::chrono::milliseconds::zero());
+        BOOST_TEST(timeouts->size() == 3);
+        BOOST_TEST((timeouts->back().second == transactions.GetDefaultTimeout()));
+    };
+
+    BOOST_TEST((transactions.GetDefaultTimeout() != std::chrono::milliseconds::zero()));
+    check();
+
+    transactions = decltype(transactions){ timeouts, std::chrono::milliseconds{ 10 } };
+    timeouts->clear();
+
+    BOOST_TEST((transactions.GetDefaultTimeout() == std::chrono::milliseconds{ 10 }));
+    check();
+}
+
 BOOST_AUTO_TEST_SUITE_END()