From b8e59c47ddd86e69d0f20508c7111b46706028a4 Mon Sep 17 00:00:00 2001 From: Song Guo Date: Mon, 27 Dec 2021 12:30:12 +0800 Subject: [PATCH] [IM] Move ownership of WriteClient to application --- src/app/DeviceProxy.cpp | 20 -- src/app/DeviceProxy.h | 3 - src/app/InteractionModelEngine.cpp | 47 ----- src/app/InteractionModelEngine.h | 22 --- src/app/WriteClient.cpp | 81 ++++---- src/app/WriteClient.h | 173 +++++++----------- src/app/tests/TestWriteInteraction.cpp | 38 ++-- .../tests/integration/chip_im_initiator.cpp | 13 +- src/controller/WriteInteraction.h | 23 ++- .../python/chip/clusters/attribute.cpp | 13 +- 10 files changed, 146 insertions(+), 287 deletions(-) diff --git a/src/app/DeviceProxy.cpp b/src/app/DeviceProxy.cpp index 0e374cff5bc385..b0958115a684c3 100644 --- a/src/app/DeviceProxy.cpp +++ b/src/app/DeviceProxy.cpp @@ -68,24 +68,4 @@ void DeviceProxy::CancelIMResponseHandler(void * commandObj) mCallbacksMgr.CancelResponseCallback(transactionId, 0 /* seqNum, always 0 for IM before #6559 */); } -CHIP_ERROR DeviceProxy::SendWriteAttributeRequest(app::WriteClientHandle aHandle, Callback::Cancelable * onSuccessCallback, - Callback::Cancelable * onFailureCallback) -{ - VerifyOrReturnLogError(IsSecureConnected(), CHIP_ERROR_INCORRECT_STATE); - - CHIP_ERROR err = CHIP_NO_ERROR; - - app::WriteClient * writeClient = aHandle.Get(); - - if (onSuccessCallback != nullptr || onFailureCallback != nullptr) - { - AddIMResponseHandler(writeClient, onSuccessCallback, onFailureCallback); - } - if ((err = aHandle.SendWriteRequest(GetSecureSession().Value())) != CHIP_NO_ERROR) - { - CancelIMResponseHandler(writeClient); - } - return err; -} - } // namespace chip diff --git a/src/app/DeviceProxy.h b/src/app/DeviceProxy.h index 50684140aabb54..fd7e8275d4144e 100644 --- a/src/app/DeviceProxy.h +++ b/src/app/DeviceProxy.h @@ -53,9 +53,6 @@ class DLL_EXPORT DeviceProxy virtual CHIP_ERROR ShutdownSubscriptions() { return CHIP_ERROR_NOT_IMPLEMENTED; } - virtual CHIP_ERROR SendWriteAttributeRequest(app::WriteClientHandle aHandle, Callback::Cancelable * onSuccessCallback, - Callback::Cancelable * onFailureCallback); - virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj); // Interaction model uses the object and callback interface instead of sequence number to mark different transactions. diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 8ee3ea7e957aa1..4b8ad8c596eedf 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -112,14 +112,6 @@ void InteractionModelEngine::Shutdown() // mpActiveReadClientList = nullptr; - for (auto & writeClient : mWriteClients) - { - if (!writeClient.IsFree()) - { - writeClient.Shutdown(); - } - } - for (auto & writeHandler : mWriteHandlers) { VerifyOrDie(writeHandler.IsFree()); @@ -147,21 +139,6 @@ uint32_t InteractionModelEngine::GetNumActiveReadHandlers() const return numActive; } -uint32_t InteractionModelEngine::GetNumActiveWriteClients() const -{ - uint32_t numActive = 0; - - for (auto & writeClient : mWriteClients) - { - if (!writeClient.IsFree()) - { - numActive++; - } - } - - return numActive; -} - uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const { uint32_t numActive = 0; @@ -205,25 +182,6 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricInde return CHIP_NO_ERROR; } -CHIP_ERROR InteractionModelEngine::NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * apCallback, - const Optional & aTimedWriteTimeoutMs) -{ - apWriteClient.SetWriteClient(nullptr); - - for (auto & writeClient : mWriteClients) - { - if (!writeClient.IsFree()) - { - continue; - } - ReturnLogErrorOnFailure(writeClient.Init(mpExchangeMgr, apCallback, aTimedWriteTimeoutMs)); - apWriteClient.SetWriteClient(&writeClient); - return CHIP_NO_ERROR; - } - - return CHIP_ERROR_NO_MEMORY; -} - void InteractionModelEngine::OnDone(CommandHandler & apCommandObj) { mCommandHandlerObjs.ReleaseObject(&apCommandObj); @@ -428,11 +386,6 @@ void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec) ChipLogValueExchange(ec)); } -uint16_t InteractionModelEngine::GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const -{ - return static_cast(apWriteClient - mWriteClients); -} - uint16_t InteractionModelEngine::GetReadHandlerArrayIndex(const ReadHandler * const apReadHandler) const { return static_cast(apReadHandler - mReadHandlers); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index e52c3c2ad85003..7f9f92d5e343a9 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -111,28 +111,9 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman */ CHIP_ERROR ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId); - /** - * Retrieve a WriteClient that the SDK consumer can use to send a write. If the call succeeds, - * see WriteClient documentation for lifetime handling. - * - * The Write interaction is more like Invoke interaction (cluster specific commands) since it will include cluster specific - * payload, and may have the need to encode non-scalar values (like structs and arrays). Thus we use WriteClientHandle to - * prevent user's code from leaking WriteClients. - * - * @param[out] apWriteClient A pointer to the WriteClient object. - * - * @retval #CHIP_ERROR_NO_MEMORY If there is no WriteClient available - * @retval #CHIP_NO_ERROR On success. - */ - CHIP_ERROR NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * callback, - const Optional & aTimedWriteTimeoutMs = NullOptional); - uint32_t GetNumActiveReadHandlers() const; uint32_t GetNumActiveWriteHandlers() const; - uint32_t GetNumActiveWriteClients() const; - - uint16_t GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const; uint16_t GetReadHandlerArrayIndex(const ReadHandler * const apReadHandler) const; /** @@ -256,12 +237,9 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman CommandHandlerInterface * mCommandHandlerList = nullptr; - // TODO(#8006): investgate if we can disable some IM functions on some compact accessories. - // TODO(#8006): investgate if we can provide more flexible object management on devices with more resources. BitMapObjectPool mCommandHandlerObjs; BitMapObjectPool mTimedHandlers; ReadHandler mReadHandlers[CHIP_IM_MAX_NUM_READ_HANDLER]; - WriteClient mWriteClients[CHIP_IM_MAX_NUM_WRITE_CLIENT]; WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER]; reporting::Engine mReportingEngine; BitMapObjectPool mClusterInfoPool; diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index fb13647938d53a..a105e550ffede5 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -48,7 +48,7 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac ReturnErrorOnFailure(mWriteRequestBuilder.GetError()); mWriteRequestBuilder.CreateWriteRequests(); ReturnErrorOnFailure(mWriteRequestBuilder.GetError()); - ClearExistingExchangeContext(); + mpExchangeMgr = apExchangeMgr; mpCallback = apCallback; mTimedWriteTimeoutMs = aTimedWriteTimeoutMs; @@ -57,30 +57,45 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac return CHIP_NO_ERROR; } -void WriteClient::Shutdown() +void WriteClient::Close() { - VerifyOrReturn(mState != State::Uninitialized); - ClearExistingExchangeContext(); - ShutdownInternal(); -} + MoveToState(State::AwaitingDestruction); -void WriteClient::ShutdownInternal() -{ - mMessageWriter.Reset(); + // OnDone below can destroy us before we unwind all the way back into the + // exchange code and it tries to close itself. Make sure that it doesn't + // try to notify us that it's closing, since we will be dead. + // + // For more details, see #10344. + if (mpExchangeCtx != nullptr) + { + mpExchangeCtx->SetDelegate(nullptr); + } - mpExchangeMgr = nullptr; mpExchangeCtx = nullptr; - ClearState(); - mpCallback->OnDone(this); + if (mpCallback) + { + mpCallback->OnDone(this); + } } -void WriteClient::ClearExistingExchangeContext() +void WriteClient::Abort() { - // Discard any existing exchange context. Effectively we can only have one IM exchange with - // a single node at any one time. + // + // If the exchange context hasn't already been gracefully closed + // (signaled by setting it to null), then we need to forcibly + // tear it down. + // if (mpExchangeCtx != nullptr) { + // We might be a delegate for this exchange, and we don't want the + // OnExchangeClosing notification in that case. Null out the delegate + // to avoid that. + // + // TODO: This makes all sorts of assumptions about what the delegate is + // (notice the "might" above!) that might not hold in practice. We + // really need a better solution here.... + mpExchangeCtx->SetDelegate(nullptr); mpExchangeCtx->Abort(); mpExchangeCtx = nullptr; } @@ -195,6 +210,9 @@ const char * WriteClient::GetStateStr() const case State::ResponseReceived: return "ResponseReceived"; + + case State::AwaitingDestruction: + return "AwaitingDestruction"; } #endif // CHIP_DETAIL_LOGGING return "N/A"; @@ -220,10 +238,6 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System:: err = FinalizeMessage(mPendingWriteData); SuccessOrExit(err); - // Discard any existing exchange context. Effectively we can only have one exchange per WriteClient - // at any one time. - ClearExistingExchangeContext(); - // Create a new exchange context. mpExchangeCtx = mpExchangeMgr->NewContext(session, this); VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY); @@ -246,7 +260,6 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System:: if (err != CHIP_NO_ERROR) { ChipLogError(DataManagement, "Write client failed to SendWriteRequest: %s", ErrorStr(err)); - ClearExistingExchangeContext(); } else { @@ -259,8 +272,10 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System:: // Always shutdown on Group communication ChipLogDetail(DataManagement, "Closing on group Communication "); - // onDone is called - ShutdownInternal(); + // Tell the application to release the object. + // TODO: The typical user would release it reference to the write client after SendWriteRequest is returned with + // success. Destruct the WriteClient before releasing that reference is weird, need to refactor the code to avoid this. + Close(); } } @@ -329,7 +344,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang if (mState != State::AwaitingResponse) { - ShutdownInternal(); + Close(); } // Else we got a response to a Timed Request and just sent the write. @@ -345,7 +360,7 @@ void WriteClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeConte { mpCallback->OnError(this, StatusIB(Protocols::InteractionModel::Status::Failure), CHIP_ERROR_TIMEOUT); } - ShutdownInternal(); + Close(); } CHIP_ERROR WriteClient::ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAttributeStatusIB) @@ -391,24 +406,6 @@ CHIP_ERROR WriteClient::ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAt return err; } -CHIP_ERROR WriteClientHandle::SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout) -{ - CHIP_ERROR err = mpWriteClient->SendWriteRequest(session, timeout); - - // Transferring ownership of the underlying WriteClient to the IM layer. IM will manage its lifetime. - // For groupcast writes, there is no transfer of ownership since the interaction is done upon transmission of the action - if (err == CHIP_NO_ERROR) - { - // Release the WriteClient without closing it. - mpWriteClient = nullptr; - } - else - { - SetWriteClient(nullptr); - } - return err; -} - CHIP_ERROR WriteClient::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload, StatusIB & aStatusIB) { diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index e81e09d422643d..39fed9bbb1e665 100644 --- a/src/app/WriteClient.h +++ b/src/app/WriteClient.h @@ -105,6 +105,49 @@ class WriteClient : public Messaging::ExchangeDelegate virtual void OnDone(WriteClient * apWriteClient) = 0; }; + /** + * Initialize the client object. Within the lifetime + * of this instance, this method is invoked once after object + * construction until a call to Shutdown is made to terminate the + * instance. + * + * @param[in] apExchangeMgr A pointer to the ExchangeManager object. + * @param[in] apDelegate InteractionModelDelegate set by application. + * @param[in] aTimedWriteTimeoutMs If provided, do a timed write using this timeout. + * @retval #CHIP_ERROR_INCORRECT_STATE incorrect state if it is already initialized + * @retval #CHIP_NO_ERROR On success. + */ + CHIP_ERROR Init(Messaging::ExchangeManager * apExchangeMgr, Callback * apDelegate, + const Optional & aTimedWriteTimeoutMs); + + /** + * Encode an attribute value that can be directly encoded using TLVWriter::Put + */ + template + CHIP_ERROR EncodeAttributeWritePayload(const chip::app::AttributePathParams & attributePath, const T & value) + { + chip::TLV::TLVWriter * writer = nullptr; + + ReturnErrorOnFailure(PrepareAttribute(attributePath)); + VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure( + DataModel::Encode(*writer, chip::TLV::ContextTag(to_underlying(chip::app::AttributeDataIB::Tag::kData)), value)); + ReturnErrorOnFailure(FinishAttribute()); + + return CHIP_NO_ERROR; + } + + /** + * Once SendWriteRequest returns successfully, the WriteClient will + * handle calling Shutdown on itself once it decides it's done with waiting + * for a response (i.e. times out or gets a response). Client can specify + * the maximum time to wait for response (in milliseconds) via timeout parameter. + * Default timeout value will be used otherwise. + * If SendWriteRequest is never called, or the call fails, the API + * consumer is responsible for calling Shutdown on the WriteClient. + */ + CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = kImMessageTimeout); + /** * Shutdown the WriteClient. This terminates this instance * of the object and releases all held resources. @@ -115,6 +158,14 @@ class WriteClient : public Messaging::ExchangeDelegate CHIP_ERROR FinishAttribute(); TLV::TLVWriter * GetAttributeDataIBTLVWriter(); + /* + * Destructor - as part of destruction, it will abort the exchange context + * if a valid one still exists. + * + * See Abort() for details on when that might occur. + */ + virtual ~WriteClient() { Abort(); } + private: friend class TestWriteInteraction; friend class InteractionModelEngine; @@ -128,6 +179,7 @@ class WriteClient : public Messaging::ExchangeDelegate AwaitingTimedStatus, // Sent a Tiemd Request, waiting for response. AwaitingResponse, // The client has sent out the write request message ResponseReceived, // We have gotten a response after sending write request + AwaitingDestruction, // The object has completed its work and is awaiting destruction by the application. }; /** @@ -135,34 +187,6 @@ class WriteClient : public Messaging::ExchangeDelegate */ CHIP_ERROR FinalizeMessage(System::PacketBufferHandle & aPacket); - /** - * Once SendWriteRequest returns successfully, the WriteClient will - * handle calling Shutdown on itself once it decides it's done with waiting - * for a response (i.e. times out or gets a response). Client can specify - * the maximum time to wait for response (in milliseconds) via timeout parameter. - * Default timeout value will be used otherwise. - * If SendWriteRequest is never called, or the call fails, the API - * consumer is responsible for calling Shutdown on the WriteClient. - */ - CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout); - - /** - * Initialize the client object. Within the lifetime - * of this instance, this method is invoked once after object - * construction until a call to Shutdown is made to terminate the - * instance. - * - * @param[in] apExchangeMgr A pointer to the ExchangeManager object. - * @param[in] apDelegate InteractionModelDelegate set by application. - * @param[in] aTimedWriteTimeoutMs If provided, do a timed write using this timeout. - * @retval #CHIP_ERROR_INCORRECT_STATE incorrect state if it is already initialized - * @retval #CHIP_NO_ERROR On success. - */ - CHIP_ERROR Init(Messaging::ExchangeManager * apExchangeMgr, Callback * apDelegate, - const Optional & aTimedWriteTimeoutMs); - - virtual ~WriteClient() = default; - CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override; void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override; @@ -175,15 +199,23 @@ class WriteClient : public Messaging::ExchangeDelegate void MoveToState(const State aTargetState); CHIP_ERROR ProcessWriteResponseMessage(System::PacketBufferHandle && payload); CHIP_ERROR ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAttributeStatusIB); - void ClearExistingExchangeContext(); const char * GetStateStr() const; void ClearState(); /** - * Internal shutdown method that we use when we know what's going on with - * our exchange and don't need to manually close it. + * Called internally to signal the completion of all work on this object, gracefully close the + * exchange (by calling into the base class) and finally, signal to the application that it's + * safe to release this object. */ - void ShutdownInternal(); + void Close(); + + /** + * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does + * not arise during normal message processing flows that all normally call Close() above. This can only + * arise due to application-initiated destruction of the object when this object is handling receiving/sending + * message payloads. + */ + void Abort(); // Handle a message received when we are expecting a status response to a // Timed Request. The caller is assumed to have already checked that our @@ -213,82 +245,5 @@ class WriteClient : public Messaging::ExchangeDelegate Optional mTimedWriteTimeoutMs; }; -class WriteClientHandle -{ -public: - /** - * Construct an empty WriteClientHandle. - */ - WriteClientHandle() : mpWriteClient(nullptr) {} - WriteClientHandle(decltype(nullptr)) : mpWriteClient(nullptr) {} - - /** - * Construct a WriteClientHandle that takes ownership of a WriteClient from another. - */ - WriteClientHandle(WriteClientHandle && aOther) - { - mpWriteClient = aOther.mpWriteClient; - aOther.mpWriteClient = nullptr; - } - - ~WriteClientHandle() { SetWriteClient(nullptr); } - - /** - * Access a WriteClientHandle's public methods. - */ - WriteClient * operator->() const { return mpWriteClient; } - - WriteClient * Get() const { return mpWriteClient; } - - /** - * Finalize the message and send it to the desired node. The underlying write object will always be released, and the user - * should not use this object after calling this function. - * - * Note: In failure cases this will _synchronously_ invoke OnDone on the - * WriteClient::Callback before returning. - */ - CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = kImMessageTimeout); - - /** - * Encode an attribute value that can be directly encoded using TLVWriter::Put - */ - template - CHIP_ERROR EncodeAttributeWritePayload(const chip::app::AttributePathParams & attributePath, const T & value) - { - chip::TLV::TLVWriter * writer = nullptr; - - VerifyOrReturnError(mpWriteClient != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(mpWriteClient->PrepareAttribute(attributePath)); - VerifyOrReturnError((writer = mpWriteClient->GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure( - DataModel::Encode(*writer, chip::TLV::ContextTag(to_underlying(chip::app::AttributeDataIB::Tag::kData)), value)); - ReturnErrorOnFailure(mpWriteClient->FinishAttribute()); - - return CHIP_NO_ERROR; - } - - /** - * Set the internal WriteClient of the Handler, expected to be called by InteractionModelEngline only since the user - * application does not have direct access to apWriteClient. - */ - void SetWriteClient(WriteClient * apWriteClient) - { - if (mpWriteClient != nullptr) - { - mpWriteClient->Shutdown(); - } - mpWriteClient = apWriteClient; - } - -private: - friend class TestWriteInteraction; - - WriteClientHandle(const WriteClientHandle &) = delete; - WriteClientHandle & operator=(const WriteClientHandle &) = delete; - WriteClientHandle & operator=(const WriteClientHandle &&) = delete; - - WriteClient * mpWriteClient = nullptr; -}; - } // namespace app } // namespace chip diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index ee167fddb16ed9..9b953704e46c59 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -50,7 +50,7 @@ class TestWriteInteraction static void TestWriteRoundtripWithClusterObjects(nlTestSuite * apSuite, void * apContext); private: - static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClientHandle & aWriteClient); + static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient); static void AddAttributeStatus(nlTestSuite * apSuite, void * apContext, WriteHandler & aWriteHandler); static void GenerateWriteRequest(nlTestSuite * apSuite, void * apContext, bool aIsTimedWrite, System::PacketBufferHandle & aPayload); @@ -84,7 +84,7 @@ class TestWriteClientCallback : public chip::app::WriteClient::Callback int mOnDoneCalled = 0; }; -void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClientHandle & aWriteClient) +void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient) { CHIP_ERROR err = CHIP_NO_ERROR; AttributePathParams attributePathParams; @@ -92,15 +92,15 @@ void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apCo attributePathParams.mClusterId = 3; attributePathParams.mAttributeId = 4; - err = aWriteClient->PrepareAttribute(attributePathParams); + err = aWriteClient.PrepareAttribute(attributePathParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - chip::TLV::TLVWriter * writer = aWriteClient->GetAttributeDataIBTLVWriter(); + chip::TLV::TLVWriter * writer = aWriteClient.GetAttributeDataIBTLVWriter(); err = writer->PutBoolean(chip::TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)), true); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = aWriteClient->FinishAttribute(); + err = aWriteClient.FinishAttribute(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } @@ -215,26 +215,22 @@ void TestWriteInteraction::TestWriteClient(nlTestSuite * apSuite, void * apConte CHIP_ERROR err = CHIP_NO_ERROR; app::WriteClient writeClient; - app::WriteClientHandle writeClientHandle; - writeClientHandle.SetWriteClient(&writeClient); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); TestWriteClientCallback callback; err = writeClient.Init(&ctx.GetExchangeManager(), &callback, /* aTimedWriteTimeoutMs = */ NullOptional); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - AddAttributeDataIB(apSuite, apContext, writeClientHandle); + AddAttributeDataIB(apSuite, apContext, writeClient); - err = writeClientHandle.SendWriteRequest(ctx.GetSessionBobToAlice()); + err = writeClient.SendWriteRequest(ctx.GetSessionBobToAlice()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - // The internal WriteClient should be nullptr once we SendWriteRequest. - NL_TEST_ASSERT(apSuite, nullptr == writeClientHandle.mpWriteClient); GenerateWriteResponse(apSuite, apContext, buf); err = writeClient.ProcessWriteResponseMessage(std::move(buf)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - writeClient.Shutdown(); + writeClient.Close(); Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); @@ -247,23 +243,21 @@ void TestWriteInteraction::TestWriteClientGroup(nlTestSuite * apSuite, void * ap CHIP_ERROR err = CHIP_NO_ERROR; app::WriteClient writeClient; - app::WriteClientHandle writeClientHandle; - writeClientHandle.SetWriteClient(&writeClient); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); TestWriteClientCallback callback; err = writeClient.Init(&ctx.GetExchangeManager(), &callback, /* aTimedWriteTimeoutMs = */ NullOptional); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - AddAttributeDataIB(apSuite, apContext, writeClientHandle); + AddAttributeDataIB(apSuite, apContext, writeClient); SessionHandle groupSession = ctx.GetSessionBobToFriends(); NL_TEST_ASSERT(apSuite, groupSession->IsGroupSession()); - err = writeClientHandle.SendWriteRequest(groupSession); + err = writeClient.SendWriteRequest(groupSession); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - // The internal WriteClient should be shutdown once we SendWriteRequest for group. - NL_TEST_ASSERT(apSuite, nullptr == writeClientHandle.mpWriteClient); + // The WriteClient should be shutdown once we SendWriteRequest for group. + NL_TEST_ASSERT(apSuite, writeClient.mState == WriteClient::State::AwaitingDestruction); } void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apContext) @@ -339,8 +333,8 @@ void TestWriteInteraction::TestWriteRoundtripWithClusterObjects(nlTestSuite * ap err = engine->Init(&ctx.GetExchangeManager(), nullptr); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - app::WriteClientHandle writeClient; - err = engine->NewWriteClient(writeClient, &callback); + app::WriteClient writeClient; + err = writeClient.Init(engine->GetExchangeManager(), &callback, Optional::Missing()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); @@ -407,8 +401,8 @@ void TestWriteInteraction::TestWriteRoundtrip(nlTestSuite * apSuite, void * apCo err = engine->Init(&ctx.GetExchangeManager(), nullptr); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - app::WriteClientHandle writeClient; - err = engine->NewWriteClient(writeClient, &callback); + app::WriteClient writeClient; + err = writeClient.Init(engine->GetExchangeManager(), &callback, Optional::Missing()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp index e00e3b8d65576d..a7df44fa0b46bc 100644 --- a/src/app/tests/integration/chip_im_initiator.cpp +++ b/src/app/tests/integration/chip_im_initiator.cpp @@ -341,7 +341,7 @@ CHIP_ERROR SendReadRequest() return err; } -CHIP_ERROR SendWriteRequest(chip::app::WriteClientHandle & apWriteClient) +CHIP_ERROR SendWriteRequest(chip::app::WriteClient & apWriteClient) { CHIP_ERROR err = CHIP_NO_ERROR; chip::TLV::TLVWriter * writer; @@ -354,13 +354,13 @@ CHIP_ERROR SendWriteRequest(chip::app::WriteClientHandle & apWriteClient) attributePathParams.mClusterId = 3; attributePathParams.mAttributeId = 4; - SuccessOrExit(err = apWriteClient->PrepareAttribute(attributePathParams)); + SuccessOrExit(err = apWriteClient.PrepareAttribute(attributePathParams)); - writer = apWriteClient->GetAttributeDataIBTLVWriter(); + writer = apWriteClient.GetAttributeDataIBTLVWriter(); SuccessOrExit(err = writer->PutBoolean(chip::TLV::ContextTag(chip::to_underlying(chip::app::AttributeDataIB::Tag::kData)), true)); - SuccessOrExit(err = apWriteClient->FinishAttribute()); + SuccessOrExit(err = apWriteClient.FinishAttribute()); SuccessOrExit(err = apWriteClient.SendWriteRequest(gSession.Get(), gMessageTimeout)); gWriteCount++; @@ -556,8 +556,9 @@ void WriteRequestTimerHandler(chip::System::Layer * systemLayer, void * appState if (gWriteRespCount < kMaxWriteMessageCount) { - chip::app::WriteClientHandle writeClient; - err = chip::app::InteractionModelEngine::GetInstance()->NewWriteClient(writeClient, &gMockDelegate); + chip::app::WriteClient writeClient; + err = writeClient.Init(chip::app::InteractionModelEngine::GetInstance()->GetExchangeManager(), &gMockDelegate, + chip::Optional::Missing()); SuccessOrExit(err); err = SendWriteRequest(writeClient); diff --git a/src/controller/WriteInteraction.h b/src/controller/WriteInteraction.h index 4f2472b5ca27ea..3651c1338d5784 100644 --- a/src/controller/WriteInteraction.h +++ b/src/controller/WriteInteraction.h @@ -78,6 +78,7 @@ class WriteCallback final : public app::WriteClient::Callback mOnDone(apWriteClient); } + chip::Platform::Delete(apWriteClient); // Always needs to be the last call chip::Platform::Delete(this); } @@ -100,29 +101,31 @@ CHIP_ERROR WriteAttribute(const SessionHandle & sessionHandle, chip::EndpointId WriteCallback::OnErrorCallbackType onErrorCb, const Optional & aTimedWriteTimeoutMs, WriteCallback::OnDoneCallbackType onDoneCb = nullptr) { - app::WriteClientHandle handle; - + auto client = Platform::MakeUnique(); auto callback = Platform::MakeUnique(onSuccessCb, onErrorCb, onDoneCb); VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(client != nullptr, CHIP_ERROR_NO_MEMORY); - ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get(), aTimedWriteTimeoutMs)); - - // At this point the handle will ensure our callback's OnDone is always - // called. - callback.release(); + ReturnErrorOnFailure( + client->Init(app::InteractionModelEngine::GetInstance()->GetExchangeManager(), callback.get(), aTimedWriteTimeoutMs)); if (sessionHandle->IsGroupSession()) { ReturnErrorOnFailure( - handle.EncodeAttributeWritePayload(chip::app::AttributePathParams(clusterId, attributeId), requestData)); + client->EncodeAttributeWritePayload(chip::app::AttributePathParams(clusterId, attributeId), requestData)); } else { ReturnErrorOnFailure( - handle.EncodeAttributeWritePayload(chip::app::AttributePathParams(endpointId, clusterId, attributeId), requestData)); + client->EncodeAttributeWritePayload(chip::app::AttributePathParams(endpointId, clusterId, attributeId), requestData)); } - ReturnErrorOnFailure(handle.SendWriteRequest(sessionHandle)); + ReturnErrorOnFailure(client->SendWriteRequest(sessionHandle)); + + // At this point the handle will ensure our callback's OnDone is always + // called. + client.release(); + callback.release(); return CHIP_NO_ERROR; } diff --git a/src/controller/python/chip/clusters/attribute.cpp b/src/controller/python/chip/clusters/attribute.cpp index 2953ae06651270..55428d23991121 100644 --- a/src/controller/python/chip/clusters/attribute.cpp +++ b/src/controller/python/chip/clusters/attribute.cpp @@ -213,7 +213,7 @@ class WriteClientCallback : public WriteClient::Callback void OnDone(WriteClient * apWriteClient) override { gOnWriteDoneCallback(mAppContext); - // delete apWriteClient; + delete apWriteClient; delete this; }; @@ -256,14 +256,14 @@ chip::ChipError::StorageType pychip_WriteClient_WriteAttributes(void * appContex CHIP_ERROR err = CHIP_NO_ERROR; std::unique_ptr callback = std::make_unique(appContext); - app::WriteClientHandle client; + std::unique_ptr client = std::make_unique(); va_list args; va_start(args, n); - SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewWriteClient( - client, callback.get(), - timedWriteTimeoutMs != 0 ? Optional(timedWriteTimeoutMs) : Optional::Missing())); + VerifyOrExit(device != nullptr && device->GetSecureSession().HasValue(), err = CHIP_ERROR_INCORRECT_STATE); + SuccessOrExit(client->Init(app::InteractionModelEngine::GetInstance()->GetExchangeManager(), callback.get(), + timedWriteTimeoutMs != 0 ? Optional(timedWriteTimeoutMs) : Optional::Missing())); { for (size_t i = 0; i < n; i++) @@ -292,8 +292,9 @@ chip::ChipError::StorageType pychip_WriteClient_WriteAttributes(void * appContex } } - SuccessOrExit(err = device->SendWriteAttributeRequest(std::move(client), nullptr, nullptr)); + SuccessOrExit(err = client->SendWriteRequest(device->GetSecureSession().Value())); + client.release(); callback.release(); exit: