Bootstrap shutdown (aws#112)

graebm · web-flow · commit 9046dd5f39ec · 2020-04-30T08:58:48.000-07:00
Allow user to know when `ClientBootstrap` is done shutting down its behind-the-scenes resources.

User may register a `std::function&lt;void()&gt;` that fires upon shutdown complete. I considered offering a `std::future` to users, but realized you can only do blocking wait()/get() operations on a c++ future, you can't register a callback to fire (a feature available in Python and Java's future primitives). So if a user wants to use `std::future` they can hook that up themselves with the callback (I did this in a test).

Also added an `EnableBlockingShutdown()` function. I was looking around at how async-shutdown was handled in other `aws-crt-cpp` classes and saw this feature on the `HttpClientConnectionManager`. It's simplifies things when the IO classes are created on the main thread's stack (all of our tests and samples).

I chose to make these functions you call on the constructed class, rather than constructor arguments, or fields in an "Options" struct for 2 reasons: 1) This class wasn't using an Options struct before, so it felt weird to add one now. 2) If these were overloaded constructor args, I feared a user would enable the "blocking shutdown" by copy/pasting a constructor with `..., true, ...` in it and later have no idea why they occasionally dead-locked.
diff --git a/include/aws/crt/io/Bootstrap.h b/include/aws/crt/io/Bootstrap.h
@@ -22,12 +22,25 @@
 #include <aws/io/channel_bootstrap.h>
 #include <aws/io/host_resolver.h>
 
+#include <future>
+
 namespace Aws
 {
     namespace Crt
     {
         namespace Io
         {
+            using OnClientBootstrapShutdownComplete = std::function<void()>;
+
+            /**
+             * A ClientBootstrap handles creation and setup of socket connections
+             * to specific endpoints.
+             *
+             * Note that ClientBootstrap may not clean up all its behind-the-scenes
+             * resources immediately upon destruction. If you need to know when
+             * behind-the-scenes shutdown is complete, use SetShutdownCompleteCallback()
+             * or EnableBlockingShutdown() (only safe on main thread).
+             */
             class AWS_CRT_CPP_API ClientBootstrap final
             {
               public:
@@ -44,11 +57,36 @@ namespace Aws
                 operator bool() const noexcept;
                 int LastError() const noexcept;
 
+                /**
+                 * Set function to invoke when ClientBootstrap's behind-the-scenes
+                 * resources finish shutting down. This function may be invoked
+                 * on any thread. Shutdown begins when the ClientBootstrap's
+                 * destructor runs.
+                 */
+                void SetShutdownCompleteCallback(OnClientBootstrapShutdownComplete callback);
+
+                /**
+                 * Force the ClientBootstrap's destructor to block until all
+                 * behind-the-scenes resources finish shutting down.
+                 *
+                 * This isn't necessary during the normal flow of an application,
+                 * but it is useful for scenarios, such as tests, that need deterministic
+                 * shutdown ordering. Be aware, if you use this anywhere other
+                 * than the main thread, YOU WILL MOST LIKELY CAUSE A DEADLOCK.
+                 *
+                 * Use SetShutdownCompleteCallback() for a thread-safe way to
+                 * know when shutdown is complete.
+                 */
+                void EnableBlockingShutdown() noexcept;
+
                 aws_client_bootstrap *GetUnderlyingHandle() const noexcept;
 
               private:
                 aws_client_bootstrap *m_bootstrap;
                 int m_lastError;
+                std::unique_ptr<struct ClientBootstrapCallbackData> m_callbackData;
+                std::future<void> m_shutdownFuture;
+                bool m_enableBlockingShutdown;
             };
         } // namespace Io
     }     // namespace Crt
diff --git a/source/io/Bootstrap.cpp b/source/io/Bootstrap.cpp
@@ -20,35 +20,74 @@ namespace Aws
     {
         namespace Io
         {
+
+            // Holds the bootstrap's shutdown promise.
+            // Lives until the bootstrap's shutdown-complete callback fires.
+            struct ClientBootstrapCallbackData
+            {
+                std::promise<void> ShutdownPromise;
+                OnClientBootstrapShutdownComplete ShutdownCallback;
+
+                static void OnShutdownComplete(void *userData)
+                {
+                    auto callbackData = static_cast<ClientBootstrapCallbackData *>(userData);
+
+                    callbackData->ShutdownPromise.set_value();
+                    callbackData->ShutdownCallback();
+
+                    delete callbackData;
+                }
+            };
+
             ClientBootstrap::ClientBootstrap(
                 EventLoopGroup &elGroup,
                 HostResolver &resolver,
                 Allocator *allocator) noexcept
-                : m_lastError(AWS_ERROR_SUCCESS)
+                : m_bootstrap(nullptr), m_lastError(AWS_ERROR_SUCCESS),
+                  m_callbackData(new ClientBootstrapCallbackData()), m_enableBlockingShutdown(false)
             {
+                m_shutdownFuture = m_callbackData->ShutdownPromise.get_future();
+
                 aws_client_bootstrap_options options;
                 options.event_loop_group = elGroup.GetUnderlyingHandle();
                 options.host_resolution_config = resolver.GetConfig();
                 options.host_resolver = resolver.GetUnderlyingHandle();
-                options.on_shutdown_complete = NULL;
-                options.user_data = NULL;
+                options.on_shutdown_complete = ClientBootstrapCallbackData::OnShutdownComplete;
+                options.user_data = m_callbackData.get();
                 m_bootstrap = aws_client_bootstrap_new(allocator, &options);
+                if (!m_bootstrap)
+                {
+                    m_lastError = aws_last_error();
+                }
             }
 
             ClientBootstrap::~ClientBootstrap()
             {
                 if (m_bootstrap)
                 {
+                    // Release m_callbackData, it destroys itself when shutdown completes.
+                    m_callbackData.release();
+
                     aws_client_bootstrap_release(m_bootstrap);
-                    m_bootstrap = nullptr;
-                    m_lastError = AWS_ERROR_UNKNOWN;
+                    if (m_enableBlockingShutdown)
+                    {
+                        // If your program is stuck here, stop using EnableBlockingShutdown()
+                        m_shutdownFuture.wait();
+                    }
                 }
             }
 
             ClientBootstrap::operator bool() const noexcept { return m_lastError == AWS_ERROR_SUCCESS; }
 
             int ClientBootstrap::LastError() const noexcept { return m_lastError; }
 
+            void ClientBootstrap::SetShutdownCompleteCallback(OnClientBootstrapShutdownComplete callback)
+            {
+                m_callbackData->ShutdownCallback = std::move(callback);
+            }
+
+            void ClientBootstrap::EnableBlockingShutdown() noexcept { m_enableBlockingShutdown = true; }
+
             aws_client_bootstrap *ClientBootstrap::GetUnderlyingHandle() const noexcept
             {
                 if (*this)
diff --git a/tests/ChannelBootstrapTest.cpp b/tests/ChannelBootstrapTest.cpp
@@ -15,6 +15,7 @@
 #include <aws/crt/Api.h>
 #include <aws/testing/aws_test_harness.h>
 
+#include <future>
 #include <utility>
 
 static int s_TestClientBootstrapResourceSafety(struct aws_allocator *allocator, void *)
@@ -29,9 +30,17 @@ static int s_TestClientBootstrapResourceSafety(struct aws_allocator *allocator,
     ASSERT_TRUE(defaultHostResolver);
     ASSERT_NOT_NULL(defaultHostResolver.GetUnderlyingHandle());
 
-    Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
-    ASSERT_TRUE(clientBootstrap);
-    ASSERT_NOT_NULL(clientBootstrap.GetUnderlyingHandle());
+    std::promise<void> bootstrapShutdownPromise;
+    std::future<void> bootstrapShutdownFuture = bootstrapShutdownPromise.get_future();
+    {
+        Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
+        ASSERT_TRUE(clientBootstrap);
+        ASSERT_NOT_NULL(clientBootstrap.GetUnderlyingHandle());
+        clientBootstrap.EnableBlockingShutdown();
+        clientBootstrap.SetShutdownCompleteCallback([&]() { bootstrapShutdownPromise.set_value(); });
+    }
+
+    ASSERT_TRUE(std::future_status::ready == bootstrapShutdownFuture.wait_for(std::chrono::seconds(10)));
 
     return AWS_ERROR_SUCCESS;
 }
diff --git a/tests/CredentialsTest.cpp b/tests/CredentialsTest.cpp
@@ -149,6 +149,7 @@ static int s_TestProviderImdsGet(struct aws_allocator *allocator, void *ctx)
 
         Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
         ASSERT_TRUE(clientBootstrap);
+        clientBootstrap.EnableBlockingShutdown();
 
         CredentialsProviderImdsConfig config;
         config.Bootstrap = &clientBootstrap;
@@ -178,6 +179,7 @@ static int s_TestProviderDefaultChainGet(struct aws_allocator *allocator, void *
 
         Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
         ASSERT_TRUE(clientBootstrap);
+        clientBootstrap.EnableBlockingShutdown();
 
         CredentialsProviderChainDefaultConfig config;
         config.Bootstrap = &clientBootstrap;
diff --git a/tests/HttpClientConnectionManagerTest.cpp b/tests/HttpClientConnectionManagerTest.cpp
@@ -56,6 +56,7 @@ static int s_TestHttpClientConnectionManagerResourceSafety(struct aws_allocator
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(clientBootstrap);
+    clientBootstrap.EnableBlockingShutdown();
 
     std::condition_variable semaphore;
     std::mutex semaphoreLock;
@@ -158,8 +159,9 @@ static int s_TestHttpClientConnectionWithPendingAcquisitions(struct aws_allocato
     Aws::Crt::Io::DefaultHostResolver defaultHostResolver(eventLoopGroup, 8, 30, allocator);
     ASSERT_TRUE(defaultHostResolver);
 
-    Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
+    Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(clientBootstrap);
+    clientBootstrap.EnableBlockingShutdown();
 
     std::condition_variable semaphore;
     std::mutex semaphoreLock;
@@ -270,6 +272,7 @@ static int s_TestHttpClientConnectionWithPendingAcquisitionsAndClosedConnections
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(clientBootstrap);
+    clientBootstrap.EnableBlockingShutdown();
 
     std::condition_variable semaphore;
     std::mutex semaphoreLock;
diff --git a/tests/HttpClientTest.cpp b/tests/HttpClientTest.cpp
@@ -99,6 +99,7 @@ static int s_TestHttpDownloadNoBackPressure(struct aws_allocator *allocator, voi
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(clientBootstrap);
+    clientBootstrap.EnableBlockingShutdown();
 
     std::shared_ptr<Http::HttpClientConnection> connection(nullptr);
     bool errorOccured = true;
@@ -232,6 +233,7 @@ static int s_TestHttpStreamUnActivated(struct aws_allocator *allocator, void *ct
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(clientBootstrap);
+    clientBootstrap.EnableBlockingShutdown();
 
     std::shared_ptr<Http::HttpClientConnection> connection(nullptr);
     bool errorOccured = true;
diff --git a/tests/IotServiceTest.cpp b/tests/IotServiceTest.cpp
@@ -67,6 +67,7 @@ static int s_TestIotPublishSubscribe(Aws::Crt::Allocator *allocator, void *ctx)
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(allocator);
+    clientBootstrap.EnableBlockingShutdown();
 
     Aws::Crt::Mqtt::MqttClient mqttClient(clientBootstrap, allocator);
     ASSERT_TRUE(mqttClient);
diff --git a/tests/MqttClientTest.cpp b/tests/MqttClientTest.cpp
@@ -37,6 +37,7 @@ static int s_TestMqttClientResourceSafety(Aws::Crt::Allocator *allocator, void *
 
     Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
     ASSERT_TRUE(allocator);
+    clientBootstrap.EnableBlockingShutdown();
 
     Aws::Crt::Mqtt::MqttClient mqttClient(clientBootstrap, allocator);
     ASSERT_TRUE(mqttClient);
diff --git a/tests/Sigv4SigningTest.cpp b/tests/Sigv4SigningTest.cpp
@@ -136,6 +136,7 @@ static int s_Sigv4SigningTestCreateDestroy(struct aws_allocator *allocator, void
 
         Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
         ASSERT_TRUE(clientBootstrap);
+        clientBootstrap.EnableBlockingShutdown();
 
         CredentialsProviderChainDefaultConfig config;
         config.Bootstrap = &clientBootstrap;
@@ -164,6 +165,7 @@ static int s_Sigv4SigningTestSimple(struct aws_allocator *allocator, void *ctx)
 
         Aws::Crt::Io::ClientBootstrap clientBootstrap(eventLoopGroup, defaultHostResolver, allocator);
         ASSERT_TRUE(clientBootstrap);
+        clientBootstrap.EnableBlockingShutdown();
 
         CredentialsProviderChainDefaultConfig config;
         config.Bootstrap = &clientBootstrap;
