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.

Sign up for GitHub

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

[IM] Move lifecycle management of WriteClient to application #13248

Merged
merged 3 commits into from
Jan 20, 2022
Merged

[IM] Move lifecycle management of WriteClient to application #13248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8e59c4
[IM] Move ownership of WriteClient to application
erjiaqing Dec 27, 2021
2b63d89
Update src/app/WriteClient.cpp
erjiaqing Jan 20, 2022
98ef9dc
Move Init to private
erjiaqing Jan 20, 2022
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
20 changes: 0 additions & 20 deletions src/app/DeviceProxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 0 additions & 3 deletions src/app/DeviceProxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
47 changes: 0 additions & 47 deletions src/app/InteractionModelEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<uint16_t> & 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);
Expand Down Expand Up @@ -428,11 +386,6 @@ void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec)
ChipLogValueExchange(ec));
}

uint16_t InteractionModelEngine::GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const
{
return static_cast<uint16_t>(apWriteClient - mWriteClients);
}

uint16_t InteractionModelEngine::GetReadHandlerArrayIndex(const ReadHandler * const apReadHandler) const
{
return static_cast<uint16_t>(apReadHandler - mReadHandlers);
Expand Down
22 changes: 0 additions & 22 deletions src/app/InteractionModelEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint16_t> & 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;
/**
Expand Down Expand Up @@ -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<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> 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<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS> mClusterInfoPool;
Expand Down
81 changes: 39 additions & 42 deletions src/app/WriteClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
erjiaqing marked this conversation as resolved.
Show resolved Hide resolved
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;
}
Expand Down Expand Up @@ -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";
Expand All @@ -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);
Expand All @@ -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
{
Expand All @@ -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.
erjiaqing marked this conversation as resolved.
Show resolved Hide resolved
Close();
erjiaqing marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -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.

Expand All @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down
Loading