Simplify ExchangeMessageDispatch, preventing it from dangling (#12794)

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
project-chip · Apr 14, 2022 · 8f1c4d7 · 8f1c4d7
1 parent 98b41ed
commit 8f1c4d7
Show file tree

Hide file tree

Showing 28 changed files with 85 additions and 217 deletions.
diff --git a/examples/all-clusters-app/ameba/chip_main.cmake b/examples/all-clusters-app/ameba/chip_main.cmake
@@ -129,6 +129,7 @@ list(
     ${chip_dir}/examples/all-clusters-app/ameba/main/CHIPDeviceManager.cpp
     ${chip_dir}/examples/all-clusters-app/ameba/main/Globals.cpp
     ${chip_dir}/examples/all-clusters-app/ameba/main/LEDWidget.cpp
+    ${chip_dir}/examples/all-clusters-app/ameba/main/DsoHack.cpp
 )
 
 add_library(

diff --git a/examples/all-clusters-app/ameba/main/DsoHack.cpp b/examples/all-clusters-app/ameba/main/DsoHack.cpp
@@ -0,0 +1,20 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+// This hack is needed because Ameba SDK is not linking against libstdc++ correctly.
+extern "C" {
+void * __dso_handle = 0;
+}
diff --git a/examples/all-clusters-app/ameba/main/chipinterface.cpp b/examples/all-clusters-app/ameba/main/chipinterface.cpp
@@ -40,10 +40,6 @@
 #include "Rpc.h"
 #endif
 
-extern "C" {
-void * __dso_handle = 0;
-}
-
 using namespace ::chip;
 using namespace ::chip::Credentials;
 using namespace ::chip::DeviceManager;

diff --git a/examples/lighting-app/ameba/chip_main.cmake b/examples/lighting-app/ameba/chip_main.cmake
@@ -25,6 +25,7 @@ list(
     ${chip_dir}/examples/lighting-app/ameba/main/CHIPDeviceManager.cpp
     ${chip_dir}/examples/lighting-app/ameba/main/Globals.cpp
     ${chip_dir}/examples/lighting-app/ameba/main/LEDWidget.cpp
+    ${chip_dir}/examples/lighting-app/ameba/main/DsoHack.cpp
 )
 
 add_library(

diff --git a/examples/lighting-app/ameba/main/DsoHack.cpp b/examples/lighting-app/ameba/main/DsoHack.cpp
@@ -0,0 +1,20 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+// This hack is needed because Ameba SDK is not linking against libstdc++ correctly.
+extern "C" {
+void * __dso_handle = 0;
+}
diff --git a/examples/lighting-app/ameba/main/chipinterface.cpp b/examples/lighting-app/ameba/main/chipinterface.cpp
@@ -38,10 +38,6 @@
 
 #include <lwip_netconf.h>
 
-extern "C" {
-void * __dso_handle = 0;
-}
-
 using namespace ::chip;
 using namespace ::chip::Credentials;
 using namespace ::chip::DeviceManager;

diff --git a/src/app/CASEClient.cpp b/src/app/CASEClient.cpp
@@ -38,8 +38,6 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
     Messaging::ExchangeContext * exchange = mInitParams.exchangeMgr->NewContext(session.Value(), &mCASESession);
     VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);
 
-    ReturnErrorOnFailure(mCASESession.MessageDispatch().Init(mInitParams.sessionManager));
-
     uint16_t keyID = 0;
     ReturnErrorOnFailure(mInitParams.idAllocator->Allocate(keyID));
 

diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp
@@ -161,7 +161,6 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
     ReturnErrorOnFailure(mIDAllocator->Allocate(keyID));
 
     mPairingSession.Clear();
-    ReturnErrorOnFailure(mPairingSession.MessageDispatch().Init(&mServer->GetSecureSessionManager()));
 
     if (mCommissioningTimeoutSeconds != kNoCommissioningTimeout)
     {

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -816,9 +816,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
 
     device->Init(GetControllerDeviceInitParams(), remoteDeviceId, peerAddress, fabric->GetFabricIndex());
 
-    err = device->GetPairing().MessageDispatch().Init(mSystemState->SessionMgr());
-    SuccessOrExit(err);
-
     if (params.GetPeerAddress().GetTransportType() != Transport::Type::kBle)
     {
         device->SetAddress(params.GetPeerAddress().GetIPAddress());

diff --git a/src/messaging/ApplicationExchangeDispatch.cpp b/src/messaging/ApplicationExchangeDispatch.cpp
@@ -26,19 +26,6 @@
 namespace chip {
 namespace Messaging {
 
-CHIP_ERROR ApplicationExchangeDispatch::PrepareMessage(const SessionHandle & session, PayloadHeader & payloadHeader,
-                                                       System::PacketBufferHandle && message,
-                                                       EncryptedPacketBufferHandle & preparedMessage)
-{
-    return mSessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage);
-}
-
-CHIP_ERROR ApplicationExchangeDispatch::SendPreparedMessage(const SessionHandle & session,
-                                                            const EncryptedPacketBufferHandle & preparedMessage) const
-{
-    return mSessionManager->SendPreparedMessage(session, preparedMessage);
-}
-
 bool ApplicationExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type)
 {
     // TODO: Change this check to only include the protocol and message types that are allowed

diff --git a/src/messaging/ApplicationExchangeDispatch.h b/src/messaging/ApplicationExchangeDispatch.h
@@ -26,36 +26,24 @@
 
 #include <lib/support/CodeUtils.h>
 #include <messaging/ExchangeMessageDispatch.h>
-#include <transport/SessionManager.h>
 
 namespace chip {
 namespace Messaging {
 
 class ApplicationExchangeDispatch : public ExchangeMessageDispatch
 {
 public:
-    ApplicationExchangeDispatch() {}
-
-    virtual ~ApplicationExchangeDispatch() {}
-
-    CHIP_ERROR Init(SessionManager * sessionManager)
+    static ExchangeMessageDispatch & Instance()
     {
-        ReturnErrorCodeIf(sessionManager == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-        mSessionManager = sessionManager;
-        return ExchangeMessageDispatch::Init();
+        static ApplicationExchangeDispatch instance;
+        return instance;
     }
 
-    CHIP_ERROR PrepareMessage(const SessionHandle & session, PayloadHeader & payloadHeader, System::PacketBufferHandle && message,
-                              EncryptedPacketBufferHandle & preparedMessage) override;
-    CHIP_ERROR SendPreparedMessage(const SessionHandle & session, const EncryptedPacketBufferHandle & message) const override;
-
-    SessionManager * GetSessionManager() const { return mSessionManager; }
+    ApplicationExchangeDispatch() {}
+    virtual ~ApplicationExchangeDispatch() {}
 
 protected:
     bool MessagePermitted(uint16_t protocol, uint8_t type) override;
-
-private:
-    SessionManager * mSessionManager = nullptr;
 };
 
 } // namespace Messaging

diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp
@@ -38,6 +38,7 @@
 #include <lib/support/Defer.h>
 #include <lib/support/TypeTraits.h>
 #include <lib/support/logging/CHIPLogging.h>
+#include <messaging/ApplicationExchangeDispatch.h>
 #include <messaging/ExchangeContext.h>
 #include <messaging/ExchangeMgr.h>
 #include <protocols/Protocols.h>
@@ -161,8 +162,9 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
         }
 
         // Create a new scope for `err`, to avoid shadowing warning previous `err`.
-        CHIP_ERROR err = mDispatch->SendMessage(mSession.Get(), mExchangeId, IsInitiator(), GetReliableMessageContext(),
-                                                reliableTransmissionRequested, protocolId, msgType, std::move(msgBuf));
+        CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get(), mExchangeId, IsInitiator(),
+                                               GetReliableMessageContext(), reliableTransmissionRequested, protocolId, msgType,
+                                               std::move(msgBuf));
         if (err != CHIP_NO_ERROR && IsResponseExpected())
         {
             CancelResponseTimer();
@@ -249,7 +251,8 @@ void ExchangeContextDeletor::Release(ExchangeContext * ec)
 }
 
 ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, const SessionHandle & session, bool Initiator,
-                                 ExchangeDelegate * delegate)
+                                 ExchangeDelegate * delegate) :
+    mDispatch((delegate != nullptr) ? delegate->GetMessageDispatch() : ApplicationExchangeDispatch::Instance())
 {
     VerifyOrDie(mExchangeMgr == nullptr);
 
@@ -259,22 +262,6 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
     mFlags.Set(Flags::kFlagInitiator, Initiator);
     mDelegate = delegate;
 
-    ExchangeMessageDispatch * dispatch = nullptr;
-    if (delegate != nullptr)
-    {
-        dispatch = delegate->GetMessageDispatch(em->GetReliableMessageMgr(), em->GetSessionManager());
-        if (dispatch == nullptr)
-        {
-            dispatch = &em->mDefaultExchangeDispatch;
-        }
-    }
-    else
-    {
-        dispatch = &em->mDefaultExchangeDispatch;
-    }
-    VerifyOrDie(dispatch != nullptr);
-    mDispatch = dispatch->Retain();
-
     SetDropAckDebug(false);
     SetAckPending(false);
     SetMsgRcvdFromPeer(false);
@@ -300,12 +287,6 @@ ExchangeContext::~ExchangeContext()
     DoClose(false);
     mExchangeMgr = nullptr;
 
-    if (mDispatch != nullptr)
-    {
-        mDispatch->Release();
-        mDispatch = nullptr;
-    }
-
 #if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
     ChipLogDetail(ExchangeManager, "ec-- id: " ChipLogFormatExchange, ChipLogValueExchange(this));
 #endif
@@ -451,7 +432,7 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload
     if (!IsGroupExchangeContext())
     {
         ReturnErrorOnFailure(
-            mDispatch->OnMessageReceived(messageCounter, payloadHeader, peerAddress, msgFlags, GetReliableMessageContext()));
+            mDispatch.OnMessageReceived(messageCounter, payloadHeader, peerAddress, msgFlags, GetReliableMessageContext()));
     }
 
     if (IsAckPending() && !mDelegate)

diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h
@@ -75,7 +75,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
      */
     bool IsInitiator() const;
 
-    bool IsEncryptionRequired() const { return mDispatch->IsEncryptionRequired(); }
+    bool IsEncryptionRequired() const { return mDispatch.IsEncryptionRequired(); }
 
     bool IsGroupExchangeContext() const { return (mSession && mSession.Get().IsGroupSession()); }
 
@@ -150,7 +150,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
 
     ReliableMessageContext * GetReliableMessageContext() { return static_cast<ReliableMessageContext *>(this); };
 
-    ExchangeMessageDispatch * GetMessageDispatch() { return mDispatch; }
+    ExchangeMessageDispatch & GetMessageDispatch() { return mDispatch; }
 
     SessionHandle GetSessionHandle() const { return mSession.Get(); }
     bool HasSessionHandle() const { return mSession; }
@@ -184,7 +184,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
     ExchangeDelegate * mDelegate   = nullptr;
     ExchangeManager * mExchangeMgr = nullptr;
 
-    ExchangeMessageDispatch * mDispatch = nullptr;
+    ExchangeMessageDispatch & mDispatch;
 
     SessionHolder mSession; // The connection state
     uint16_t mExchangeId;   // Assigned exchange ID.

diff --git a/src/messaging/ExchangeDelegate.h b/src/messaging/ExchangeDelegate.h
@@ -97,10 +97,7 @@ class DLL_EXPORT ExchangeDelegate
      */
     virtual void OnExchangeClosing(ExchangeContext * ec) {}
 
-    virtual ExchangeMessageDispatch * GetMessageDispatch(ReliableMessageMgr * reliableMessageMgr, SessionManager * sessionManager)
-    {
-        return nullptr;
-    }
+    virtual ExchangeMessageDispatch & GetMessageDispatch() { return ApplicationExchangeDispatch::Instance(); }
 };
 
 } // namespace Messaging

diff --git a/src/messaging/ExchangeMessageDispatch.cpp b/src/messaging/ExchangeMessageDispatch.cpp
@@ -41,9 +41,10 @@
 namespace chip {
 namespace Messaging {
 
-CHIP_ERROR ExchangeMessageDispatch::SendMessage(const SessionHandle & session, uint16_t exchangeId, bool isInitiator,
-                                                ReliableMessageContext * reliableMessageContext, bool isReliableTransmission,
-                                                Protocols::Id protocol, uint8_t type, System::PacketBufferHandle && message)
+CHIP_ERROR ExchangeMessageDispatch::SendMessage(SessionManager * sessionManager, const SessionHandle & session, uint16_t exchangeId,
+                                                bool isInitiator, ReliableMessageContext * reliableMessageContext,
+                                                bool isReliableTransmission, Protocols::Id protocol, uint8_t type,
+                                                System::PacketBufferHandle && message)
 {
     ReturnErrorCodeIf(!MessagePermitted(protocol.GetProtocolId(), type), CHIP_ERROR_INVALID_ARGUMENT);
 
@@ -82,8 +83,8 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(const SessionHandle & session, u
         };
         std::unique_ptr<ReliableMessageMgr::RetransTableEntry, decltype(deleter)> entryOwner(entry, deleter);
 
-        ReturnErrorOnFailure(PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf));
-        CHIP_ERROR err = SendPreparedMessage(session, entryOwner->retainedBuf);
+        ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf));
+        CHIP_ERROR err = sessionManager->SendPreparedMessage(session, entryOwner->retainedBuf);
         if (err == CHIP_ERROR_POSIX(ENOBUFS))
         {
             // sendmsg on BSD-based systems never blocks, no matter how the
@@ -105,8 +106,8 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(const SessionHandle & session, u
         // If the channel itself is providing reliability, let's not request MRP acks
         payloadHeader.SetNeedsAck(false);
         EncryptedPacketBufferHandle preparedMessage;
-        ReturnErrorOnFailure(PrepareMessage(session, payloadHeader, std::move(message), preparedMessage));
-        ReturnErrorOnFailure(SendPreparedMessage(session, preparedMessage));
+        ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage));
+        ReturnErrorOnFailure(sessionManager->SendPreparedMessage(session, preparedMessage));
     }
 
     return CHIP_NO_ERROR;

diff --git a/src/messaging/ExchangeMessageDispatch.h b/src/messaging/ExchangeMessageDispatch.h
@@ -23,47 +23,28 @@
 
 #pragma once
 
-#include <lib/core/ReferenceCounted.h>
 #include <messaging/Flags.h>
 #include <transport/SessionManager.h>
 
 namespace chip {
 namespace Messaging {
 
-class ReliableMessageMgr;
 class ReliableMessageContext;
 
-class ExchangeMessageDispatch : public ReferenceCounted<ExchangeMessageDispatch>
+class ExchangeMessageDispatch
 {
 public:
     ExchangeMessageDispatch() {}
     virtual ~ExchangeMessageDispatch() {}
 
-    CHIP_ERROR Init() { return CHIP_NO_ERROR; }
-
     virtual bool IsEncryptionRequired() const { return true; }
 
-    CHIP_ERROR SendMessage(const SessionHandle & session, uint16_t exchangeId, bool isInitiator,
+    CHIP_ERROR SendMessage(SessionManager * sessionManager, const SessionHandle & session, uint16_t exchangeId, bool isInitiator,
                            ReliableMessageContext * reliableMessageContext, bool isReliableTransmission, Protocols::Id protocol,
                            uint8_t type, System::PacketBufferHandle && message);
-
-    /**
-     * @brief
-     *   This interface takes the payload and returns the prepared message which can be send multiple times.
-     *
-     * @param session         Peer node to which the payload to be sent
-     * @param payloadHeader   The payloadHeader to be encoded into the packet
-     * @param message         The payload to be sent
-     * @param preparedMessage The handle to hold the prepared message
-     */
-    virtual CHIP_ERROR PrepareMessage(const SessionHandle & session, PayloadHeader & payloadHeader,
-                                      System::PacketBufferHandle && message, EncryptedPacketBufferHandle & preparedMessage) = 0;
-    virtual CHIP_ERROR SendPreparedMessage(const SessionHandle & session,
-                                           const EncryptedPacketBufferHandle & preparedMessage) const                       = 0;
-
-    virtual CHIP_ERROR OnMessageReceived(uint32_t messageCounter, const PayloadHeader & payloadHeader,
-                                         const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
-                                         ReliableMessageContext * reliableMessageContext);
+    CHIP_ERROR OnMessageReceived(uint32_t messageCounter, const PayloadHeader & payloadHeader,
+                                 const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
+                                 ReliableMessageContext * reliableMessageContext);
 
 protected:
     virtual bool MessagePermitted(uint16_t protocol, uint8_t type) = 0;

diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp
@@ -87,7 +87,6 @@ CHIP_ERROR ExchangeManager::Init(SessionManager * sessionManager)
     sessionManager->SetMessageDelegate(this);
 
     mReliableMessageMgr.Init(sessionManager->SystemLayer());
-    ReturnErrorOnFailure(mDefaultExchangeDispatch.Init(mSessionManager));
 
     mState = State::kState_Initialized;
 

diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h
@@ -232,8 +232,6 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate, public Session
     SessionManager * mSessionManager;
     ReliableMessageMgr mReliableMessageMgr;
 
-    ApplicationExchangeDispatch mDefaultExchangeDispatch;
-
     FabricIndex mFabricIndex = 0;
 
     BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;