Version 1.22.0 alexa-client-sdk

Changes in this update:

Feature enhancements, updates, and resolved issues from all releases are available on the [Amazon developer portal](https://developer.amazon.com/docs/alexa/avs-device-sdk/release-notes.html)

Author: scotthea-amazon

ACL/include/ACL/Transport/MessageRouter.h

@@ -26,6 +26,7 @@
 #include <AVSCommon/AVS/MessageRequest.h>
 #include <AVSCommon/SDKInterfaces/AuthDelegateInterface.h>
 #include <AVSCommon/Utils/Threading/Executor.h>
+#include <AVSCommon/Utils/Timing/Timer.h>
 
 #include "ACL/Transport/MessageConsumerInterface.h"
 #include "ACL/Transport/MessageRouterInterface.h"
@@ -49,6 +50,9 @@ class MessageRouter
         , public MessageConsumerInterface
         , public std::enable_shared_from_this<MessageRouter> {
 public:
+    /// Amount of time to allow for an automatic reconnect before notifying of a server side disconnect.
+    static const std::chrono::milliseconds DEFAULT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD;
+
     /**
      * Factory function for creating an instance of MessageRouterInterface.
      *
@@ -77,13 +81,16 @@ class MessageRouter
      * is used.
      * @param engineType optional parameter of engine type associated with this MessageRouter. Default to be
      * ENGINE_TYPE_ALEXA_VOICE_SERVICES.
+     * @param serverSideDisconnectGracePeriod How long to allow for an automatic reconnection before reporting
+     * a server side disconnect to our observer.
      */
     MessageRouter(
         std::shared_ptr<avsCommon::sdkInterfaces::AuthDelegateInterface> authDelegate,
         std::shared_ptr<avsCommon::avs::attachment::AttachmentManagerInterface> attachmentManager,
         std::shared_ptr<TransportFactoryInterface> transportFactory,
         const std::string& avsGateway = "",
-        int engineType = avsCommon::sdkInterfaces::ENGINE_TYPE_ALEXA_VOICE_SERVICES);
+        int engineType = avsCommon::sdkInterfaces::ENGINE_TYPE_ALEXA_VOICE_SERVICES,
+        std::chrono::milliseconds serverSideDisconnectGracePeriod = DEFAULT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD);
 
     /// @name MessageRouterInterface methods.
     /// @{
@@ -136,6 +143,16 @@ class MessageRouter
         const avsCommon::sdkInterfaces::ConnectionStatusObserverInterface::Status status,
         const avsCommon::sdkInterfaces::ConnectionStatusObserverInterface::ChangedReason reason);
 
+    /**
+     * Without any further queueing to the executor, notify the connection observer that the status has changed.
+     *
+     * @param status The current status of the connection.
+     * @param reason The reason the connection status changed.
+     */
+    void handleNotifyObserverOnConnectionStatusChanged(
+        const avsCommon::sdkInterfaces::ConnectionStatusObserverInterface::Status status,
+        const avsCommon::sdkInterfaces::ConnectionStatusObserverInterface::ChangedReason reason);
+
     /**
      * Notify the message observer of an incoming message from AVS.
      * Architectural note:
@@ -228,6 +245,18 @@ class MessageRouter
     /// The synchonized queue of messages to send that is shared between transports.
     std::shared_ptr<SynchronizedMessageRequestQueue> m_requestQueue;
 
+    /// Timer used to smooth over server side disconnects.
+    avsCommon::utils::timing::Timer m_serverSideDisconnectTimer;
+
+    /// True if notification of a server side disconnect should be delivered when the timer triggers.
+    bool m_serverSideDisconnectNotificationPending;
+
+    /// The last connection status reported to our observer.
+    avsCommon::sdkInterfaces::ConnectionStatusObserverInterface::Status m_lastReportedConnectionStatus;
+
+    /// Amount of time to allow for an automatic reconnect before notifying of a server side disconnect.
+    const std::chrono::milliseconds m_serverSideReconnectGracePeriod;
+
 protected:
     /**
      * Executor to perform asynchronous operations:

ACL/src/Transport/MessageRouter.cpp

@@ -36,6 +36,8 @@ static const std::string TAG("MessageRouter");
 /// String for logging purpose as the key for the size of m_transports.
 static constexpr const char* KEY_SIZEOF_TRANSPORTS = "sizeOf m_transports";
 
+const std::chrono::milliseconds MessageRouter::DEFAULT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD(15000);
+
 /**
  * Create a LogEntry using this file's TAG and the specified event string.
  *
@@ -74,7 +76,8 @@ MessageRouter::MessageRouter(
     std::shared_ptr<AttachmentManagerInterface> attachmentManager,
     std::shared_ptr<TransportFactoryInterface> transportFactory,
     const std::string& avsGateway,
-    int engineType) :
+    int engineType,
+    std::chrono::milliseconds serverSideDisconnectGracePeriod) :
         MessageRouterInterface{"MessageRouter"},
         m_avsGateway{avsGateway},
         m_authDelegate{authDelegate},
@@ -84,7 +87,10 @@ MessageRouter::MessageRouter(
         m_isEnabled{false},
         m_attachmentManager{attachmentManager},
         m_transportFactory{transportFactory},
-        m_requestQueue{std::make_shared<SynchronizedMessageRequestQueue>()} {
+        m_requestQueue{std::make_shared<SynchronizedMessageRequestQueue>()},
+        m_serverSideDisconnectNotificationPending{false},
+        m_lastReportedConnectionStatus{ConnectionStatusObserverInterface::Status::DISCONNECTED},
+        m_serverSideReconnectGracePeriod{serverSideDisconnectGracePeriod} {
 }
 
 MessageRouterInterface::ConnectionStatus MessageRouter::getConnectionStatus() {
@@ -258,10 +264,8 @@ void MessageRouter::onDisconnected(
 
 void MessageRouter::onServerSideDisconnect(std::shared_ptr<TransportInterface> transport) {
     std::unique_lock<std::mutex> lock{m_connectionMutex};
-    ACSDK_INFO(LX("server-side disconnect received")
-                   .d("Message router is enabled", m_isEnabled)
-                   .p("transport", transport)
-                   .p("m_activeTransport", m_activeTransport));
+    ACSDK_INFO(
+        LX(__func__).d("m_isEnabled", m_isEnabled).p("transport", transport).p("m_activeTransport", m_activeTransport));
     if (m_isEnabled && transport == m_activeTransport) {
         setConnectionStatusLocked(
             ConnectionStatusObserverInterface::Status::PENDING,
@@ -299,14 +303,47 @@ void MessageRouter::notifyObserverOnConnectionStatusChanged(
     const ConnectionStatusObserverInterface::Status status,
     const ConnectionStatusObserverInterface::ChangedReason reason) {
     auto task = [this, status, reason]() {
+        if (m_serverSideReconnectGracePeriod != std::chrono::milliseconds(0) &&
+            ConnectionStatusObserverInterface::Status::PENDING == status &&
+            ConnectionStatusObserverInterface::ChangedReason::SERVER_SIDE_DISCONNECT == reason) {
+            m_serverSideDisconnectNotificationPending = true;
+            m_serverSideDisconnectTimer.start(m_serverSideReconnectGracePeriod, [this]() {
+                ACSDK_DEBUG0(LX("serverSideDisconectTimerPredicate"));
+                m_executor.submit([this]() {
+                    ACSDK_DEBUG0(
+                        LX("serverSideDisconectTimerHandler")
+                            .d("m_serverSideDisconnectNotificationPending", m_serverSideDisconnectNotificationPending));
+                    if (m_serverSideDisconnectNotificationPending) {
+                        handleNotifyObserverOnConnectionStatusChanged(
+                            ConnectionStatusObserverInterface::Status::PENDING,
+                            ConnectionStatusObserverInterface::ChangedReason::SERVER_SIDE_DISCONNECT);
+                    }
+                });
+            });
+        } else {
+            if (m_serverSideDisconnectNotificationPending) {
+                ACSDK_DEBUG0(LX("NotificationOfServerSideDisconnectSuppressed"));
+            }
+            m_serverSideDisconnectTimer.stop();
+            handleNotifyObserverOnConnectionStatusChanged(status, reason);
+        }
+    };
+    m_executor.submit(task);
+}
+
+void MessageRouter::handleNotifyObserverOnConnectionStatusChanged(
+    const ConnectionStatusObserverInterface::Status status,
+    const ConnectionStatusObserverInterface::ChangedReason reason) {
+    m_serverSideDisconnectNotificationPending = false;
+    if (status != m_lastReportedConnectionStatus) {
+        m_lastReportedConnectionStatus = status;
         auto observer = getObserver();
         if (observer) {
             std::vector<ConnectionStatusObserverInterface::EngineConnectionStatus> statuses;
             statuses.emplace_back(m_engineType, reason, status);
             observer->onConnectionStatusChanged(status, statuses);
         }
-    };
-    m_executor.submit(task);
+    }
 }
 
 void MessageRouter::notifyObserverOnReceive(const std::string& contextId, const std::string& message) {

ACL/src/Transport/PostConnectSequencerFactory.cpp

@@ -48,7 +48,7 @@ class LegacyProviderRegistrar : public PostConnectOperationProviderRegistrarInte
      *
      * @param postConnectOperationProviders The providers to (pre) register.
      */
-    LegacyProviderRegistrar(
+    explicit LegacyProviderRegistrar(
         const std::vector<std::shared_ptr<PostConnectOperationProviderInterface>>& postConnectOperationProviders);
 
     /// @name PostConnectOperationProviderRegistrarInterface methods.

ACL/test/Transport/MessageRouterTest.cpp

@@ -158,7 +158,7 @@ TEST_F(MessageRouterTest, test_disconnectDisconnectsConnectedTransports) {
     m_router->disable();
 }
 
-TEST_F(MessageRouterTest, test_serverSideDisconnectCreatesANewTransport) {
+TEST_F(MessageRouterTest, test_serverSideDisconnectWithLongDelayedReconnectReportsPending) {
     /*
      * This test is difficult to setup in a nice way. The idea is to replace the original
      * transport with a new one, call onServerSideDisconnect to make it the new active
@@ -173,16 +173,70 @@ TEST_F(MessageRouterTest, test_serverSideDisconnectCreatesANewTransport) {
 
     m_transportFactory->setMockTransport(newTransport);
 
-    // Reset the MessageRouterObserver, there should be no interactions with the observer
+    // Trigger server side disconnect handling
     m_router->onServerSideDisconnect(oldTransport);
 
+    // Simulate delayed reconnect, waiting for the server side disconnect grace period to expire so
+    // so that we can see the transition back to the PENDING state.
+    ASSERT_TRUE(m_mockMessageRouterObserver->waitForStatusChange(
+        TestableMessageRouter::SHORT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD + SHORT_TIMEOUT_MS,
+        ConnectionStatusObserverInterface::Status::PENDING,
+        ConnectionStatusObserverInterface::ChangedReason::SERVER_SIDE_DISCONNECT))
+        << "status=" << m_mockMessageRouterObserver->getLatestConnectionStatus()
+        << "reason=" << m_mockMessageRouterObserver->getLatestConnectionChangedReason();
+
+    // mock the new transports connection
+    connectMockTransport(newTransport.get());
+    m_router->onConnected(newTransport);
+
     waitOnMessageRouter(SHORT_TIMEOUT_MS);
 
     ASSERT_EQ(
-        m_mockMessageRouterObserver->getLatestConnectionStatus(), ConnectionStatusObserverInterface::Status::PENDING);
+        m_mockMessageRouterObserver->getLatestConnectionStatus(), ConnectionStatusObserverInterface::Status::CONNECTED);
     ASSERT_EQ(
         m_mockMessageRouterObserver->getLatestConnectionChangedReason(),
-        ConnectionStatusObserverInterface::ChangedReason::SERVER_SIDE_DISCONNECT);
+        ConnectionStatusObserverInterface::ChangedReason::ACL_CLIENT_REQUEST);
+
+    // mock the old transport disconnecting completely
+    disconnectMockTransport(oldTransport.get());
+    m_router->onDisconnected(oldTransport, ConnectionStatusObserverInterface::ChangedReason::ACL_CLIENT_REQUEST);
+
+    auto messageRequest = createMessageRequest();
+
+    EXPECT_CALL(*oldTransport.get(), onRequestEnqueued()).Times(0);
+
+    EXPECT_CALL(*newTransport.get(), onRequestEnqueued()).Times(1);
+
+    m_router->sendMessage(messageRequest);
+
+    waitOnMessageRouter(SHORT_TIMEOUT_MS);
+}
+
+TEST_F(MessageRouterTest, test_serverSideDisconnectWithReconnectDoesNotReportingPending) {
+    /*
+     * This test is difficult to setup in a nice way. The idea is to replace the original
+     * transport with a new one, call onServerSideDisconnect to make it the new active
+     * transport, and then send a message. The message should be sent on the new transport.
+     */
+    setupStateToConnected();
+
+    auto oldTransport = m_mockTransport;
+
+    auto newTransport = std::make_shared<NiceMock<MockTransport>>();
+    initializeMockTransport(newTransport.get());
+
+    m_transportFactory->setMockTransport(newTransport);
+
+    // Trigger server side disconnect handling
+    m_router->onServerSideDisconnect(oldTransport);
+
+    // Simulate slightly delayed reconnect, verifying that no transition to PENDING is reported.
+    ASSERT_FALSE(m_mockMessageRouterObserver->waitForStatusChange(
+        SHORT_TIMEOUT_MS,
+        ConnectionStatusObserverInterface::Status::PENDING,
+        ConnectionStatusObserverInterface::ChangedReason::SERVER_SIDE_DISCONNECT))
+        << "status=" << m_mockMessageRouterObserver->getLatestConnectionStatus()
+        << "reason=" << m_mockMessageRouterObserver->getLatestConnectionChangedReason();
 
     // mock the new transports connection
     connectMockTransport(newTransport.get());

ACL/test/Transport/MessageRouterTest.h

@@ -50,7 +50,13 @@ class TestableMessageRouter : public MessageRouter {
         std::shared_ptr<AttachmentManagerInterface> attachmentManager,
         std::shared_ptr<TransportFactoryInterface> factory,
         const std::string& avsGateway) :
-            MessageRouter(authDelegate, attachmentManager, factory, avsGateway) {
+            MessageRouter(
+                authDelegate,
+                attachmentManager,
+                factory,
+                avsGateway,
+                avsCommon::sdkInterfaces::ENGINE_TYPE_ALEXA_VOICE_SERVICES,
+                SHORT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD) {
     }
 
     /**
@@ -64,11 +70,14 @@ class TestableMessageRouter : public MessageRouter {
         auto status = future.wait_for(millisecondsToWait);
         return status == std::future_status::ready;
     }
+
+    /// Short amount of time to allow for an automatic reconnect before notifying of a server side disconnect.
+    static const std::chrono::milliseconds SHORT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD;
 };
 
 class MockTransportFactory : public TransportFactoryInterface {
 public:
-    MockTransportFactory(std::shared_ptr<MockTransport> transport) : m_mockTransport{transport} {
+    explicit MockTransportFactory(std::shared_ptr<MockTransport> transport) : m_mockTransport{transport} {
     }
 
     void setMockTransport(std::shared_ptr<MockTransport> transport) {
@@ -146,6 +155,7 @@ class MessageRouterTest : public ::testing::Test {
     // TestableMessageRouter m_router;
 };
 
+const std::chrono::milliseconds TestableMessageRouter::SHORT_SERVER_SIDE_DISCONNECT_GRACE_PERIOD(2000);
 const std::string MessageRouterTest::MESSAGE = "123456789";
 const int MessageRouterTest::MESSAGE_LENGTH = 10;
 const std::chrono::milliseconds MessageRouterTest::SHORT_TIMEOUT_MS = std::chrono::milliseconds(1000);

ACL/test/Transport/MockHTTP2Request.h

@@ -39,7 +39,7 @@ namespace test {
  */
 class MockHTTP2Request : public HTTP2RequestInterface {
 public:
-    MockHTTP2Request(const alexaClientSDK::avsCommon::utils::http2::HTTP2RequestConfig& config);
+    explicit MockHTTP2Request(const alexaClientSDK::avsCommon::utils::http2::HTTP2RequestConfig& config);
     MOCK_METHOD0(cancel, bool());
     MOCK_CONST_METHOD0(getId, std::string());