Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify ExchangeMessageDispatch, preventing it from dangling #12794

Merged
merged 2 commits into from
Dec 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/all-clusters-app/ameba/chip_main.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
20 changes: 20 additions & 0 deletions examples/all-clusters-app/ameba/main/DsoHack.cpp
Original file line number Diff line number Diff line change
@@ -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;
}
4 changes: 0 additions & 4 deletions examples/all-clusters-app/ameba/main/chipinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions examples/lighting-app/ameba/chip_main.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
20 changes: 20 additions & 0 deletions examples/lighting-app/ameba/main/DsoHack.cpp
Original file line number Diff line number Diff line change
@@ -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;
}
4 changes: 0 additions & 4 deletions examples/lighting-app/ameba/main/chipinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions src/app/CASEClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
1 change: 0 additions & 1 deletion src/app/server/CommissioningWindowManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
ReturnErrorOnFailure(mIDAllocator->Allocate(keyID));

mPairingSession.Clear();
ReturnErrorOnFailure(mPairingSession.MessageDispatch().Init(&mServer->GetSecureSessionManager()));

if (mCommissioningTimeoutSeconds != kNoCommissioningTimeout)
{
Expand Down
3 changes: 0 additions & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
13 changes: 0 additions & 13 deletions src/messaging/ApplicationExchangeDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 5 additions & 17 deletions src/messaging/ApplicationExchangeDispatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 7 additions & 26 deletions src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -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()); }

Expand Down Expand Up @@ -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; }
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 1 addition & 4 deletions src/messaging/ExchangeDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions src/messaging/ExchangeMessageDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down
29 changes: 5 additions & 24 deletions src/messaging/ExchangeMessageDispatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion src/messaging/ExchangeMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 0 additions & 2 deletions src/messaging/ExchangeMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading