From f0a5e86be452c5d068eda882976e7bab2b3cfac9 Mon Sep 17 00:00:00 2001 From: Song GUO Date: Thu, 20 Jan 2022 14:43:51 +0800 Subject: [PATCH] [IM] Move lifecycle management of WriteClient to application (#13248) * [IM] Move ownership of WriteClient to application * Update src/app/WriteClient.cpp Co-authored-by: Boris Zbarsky * Move Init to private Co-authored-by: Boris Zbarsky --- src/app/DeviceProxy.cpp | 20 -- src/app/DeviceProxy.h | 3 - src/app/InteractionModelEngine.cpp | 47 ----- src/app/InteractionModelEngine.h | 22 --- src/app/WriteClient.cpp | 98 +++++----- src/app/WriteClient.h | 175 +++++++----------- src/app/tests/TestWriteInteraction.cpp | 50 ++--- .../tests/integration/chip_im_initiator.cpp | 12 +- src/controller/WriteInteraction.h | 24 +-- .../python/chip/clusters/attribute.cpp | 13 +- 10 files changed, 159 insertions(+), 305 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..fcb647adb8922b 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -31,11 +31,10 @@ namespace chip { namespace app { -CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, - const Optional & aTimedWriteTimeoutMs) +CHIP_ERROR WriteClient::Init() { - VerifyOrReturnError(apExchangeMgr != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(mpExchangeMgr == nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::Uninitialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mpExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mpExchangeCtx == nullptr, CHIP_ERROR_INCORRECT_STATE); System::PacketBufferHandle packet = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes); @@ -44,43 +43,55 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac mMessageWriter.Init(std::move(packet)); ReturnErrorOnFailure(mWriteRequestBuilder.Init(&mMessageWriter)); - mWriteRequestBuilder.TimedRequest(aTimedWriteTimeoutMs.HasValue()); + mWriteRequestBuilder.TimedRequest(mTimedWriteTimeoutMs.HasValue()); ReturnErrorOnFailure(mWriteRequestBuilder.GetError()); mWriteRequestBuilder.CreateWriteRequests(); ReturnErrorOnFailure(mWriteRequestBuilder.GetError()); - ClearExistingExchangeContext(); - mpExchangeMgr = apExchangeMgr; - mpCallback = apCallback; - mTimedWriteTimeoutMs = aTimedWriteTimeoutMs; + MoveToState(State::Initialized); 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; } @@ -135,6 +146,10 @@ CHIP_ERROR WriteClient::ProcessWriteResponseMessage(System::PacketBufferHandle & CHIP_ERROR WriteClient::PrepareAttribute(const AttributePathParams & attributePathParams) { + if (mState == State::Uninitialized) + { + ReturnErrorOnFailure(Init()); + } VerifyOrReturnError(attributePathParams.IsValidAttributePath(), CHIP_ERROR_INVALID_PATH_LIST); AttributeDataIBs::Builder & writeRequests = mWriteRequestBuilder.GetWriteRequests(); AttributeDataIB::Builder & attributeDataIB = writeRequests.CreateAttributeDataIBBuilder(); @@ -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,11 @@ 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: Consumers expect to hand off ownership of the WriteClient and wait for OnDone + // after SendWriteRequest returns success. Calling OnDone before returning is weird. + // Need to refactor the code to avoid this. + Close(); } } @@ -329,7 +345,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 +361,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 +407,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..43ab35df860148 100644 --- a/src/app/WriteClient.h +++ b/src/app/WriteClient.h @@ -105,6 +105,48 @@ class WriteClient : public Messaging::ExchangeDelegate virtual void OnDone(WriteClient * apWriteClient) = 0; }; + /** + * Construct the client object. Within the lifetime + * of this 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. + */ + WriteClient(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, + const Optional & aTimedWriteTimeoutMs) : + mpExchangeMgr(apExchangeMgr), + mpCallback(apCallback), mTimedWriteTimeoutMs(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 +157,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,40 +178,18 @@ 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. }; /** - * Finalize Write Request Message TLV Builder and retrieve final data from tlv builder for later sending + * The actual init function, called during encoding first attribute data. */ - CHIP_ERROR FinalizeMessage(System::PacketBufferHandle & aPacket); + CHIP_ERROR Init(); /** - * 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. + * Finalize Write Request Message TLV Builder and retrieve final data from tlv builder for later sending */ - CHIP_ERROR Init(Messaging::ExchangeManager * apExchangeMgr, Callback * apDelegate, - const Optional & aTimedWriteTimeoutMs); - - virtual ~WriteClient() = default; + CHIP_ERROR FinalizeMessage(System::PacketBufferHandle & aPacket); CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override; @@ -175,15 +203,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 +249,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..57b893372007de 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); } @@ -214,27 +214,21 @@ void TestWriteInteraction::TestWriteClient(nlTestSuite * apSuite, void * apConte CHIP_ERROR err = CHIP_NO_ERROR; - app::WriteClient writeClient; - app::WriteClientHandle writeClientHandle; - writeClientHandle.SetWriteClient(&writeClient); + TestWriteClientCallback callback; + app::WriteClient writeClient(&ctx.GetExchangeManager(), &callback, /* aTimedWriteTimeoutMs = */ NullOptional); 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); @@ -246,24 +240,20 @@ void TestWriteInteraction::TestWriteClientGroup(nlTestSuite * apSuite, void * ap CHIP_ERROR err = CHIP_NO_ERROR; - app::WriteClient writeClient; - app::WriteClientHandle writeClientHandle; - writeClientHandle.SetWriteClient(&writeClient); + TestWriteClientCallback callback; + app::WriteClient writeClient(&ctx.GetExchangeManager(), &callback, /* aTimedWriteTimeoutMs = */ NullOptional); 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,9 +329,7 @@ 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); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional::Missing()); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); @@ -407,9 +395,7 @@ 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); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional::Missing()); System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); AddAttributeDataIB(apSuite, apContext, writeClient); diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp index e00e3b8d65576d..7e84ac02f8e64d 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,8 @@ 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(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..09cc527f462515 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,30 @@ CHIP_ERROR WriteAttribute(const SessionHandle & sessionHandle, chip::EndpointId WriteCallback::OnErrorCallbackType onErrorCb, const Optional & aTimedWriteTimeoutMs, WriteCallback::OnDoneCallbackType onDoneCb = nullptr) { - app::WriteClientHandle handle; - auto callback = Platform::MakeUnique(onSuccessCb, onErrorCb, onDoneCb); - VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); - - ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get(), aTimedWriteTimeoutMs)); + auto client = Platform::MakeUnique(app::InteractionModelEngine::GetInstance()->GetExchangeManager(), + callback.get(), aTimedWriteTimeoutMs); - // At this point the handle will ensure our callback's OnDone is always - // called. - callback.release(); + VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(client != nullptr, CHIP_ERROR_NO_MEMORY); 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..465448437aa7ee 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( + app::InteractionModelEngine::GetInstance()->GetExchangeManager(), callback.get(), + timedWriteTimeoutMs != 0 ? Optional(timedWriteTimeoutMs) : Optional::Missing()); 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); { 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: