Skip to content

Commit

Permalink
Upon running out of space in InvokeResponseMessage for additional res…
Browse files Browse the repository at this point in the history 
…ponses, allocate another (#31516)


Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
@tehampson @bzbarsky-apple @pull
2 people authored and pull[bot] committed May 6, 2024
1 parent fef6e55 commit 4431191
Show file tree
Hide file tree
Showing 5 changed files with 342 additions and 57 deletions.
68 changes: 63 additions & 5 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
ReturnErrorOnFailure(mInvokeResponseBuilder.GetError());

mBufferAllocated = true;
MoveToState(State::NewResponseMessage);
}

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -572,7 +573,7 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman
return Status::Success;
}

CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus)
CHIP_ERROR CommandHandler::TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus)
{
ReturnErrorOnFailure(PrepareStatus(aCommandPath));
CommandStatusIB::Builder & commandStatus = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus();
Expand All @@ -583,6 +584,11 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman
return FinishStatus();
}

CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus)
{
return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddStatusInternal(aCommandPath, aStatus); });
}

void CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
const char * context)
{
Expand Down Expand Up @@ -655,12 +661,22 @@ CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistr
{
ReturnErrorOnFailure(AllocateBuffer());

mInvokeResponseBuilder.Checkpoint(mBackupWriter);
mBackupState = mState;
if (!mInternalCallToAddResponseData && mState == State::AddedCommand)
{
// An attempt is being made to add CommandData InvokeResponse using primitive
// CommandHandler APIs. While not recommended, as this potentially leaves the
// CommandHandler in an incorrect state upon failure, this approach is permitted
// for legacy reasons. To maximize the likelihood of success, particularly when
// handling large amounts of data, we try to obtain a new, completely empty
// InvokeResponseMessage, as the existing one already has space occupied.
ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext());
}

CreateBackupForResponseRollback();
//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mState == State::NewResponseMessage || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);

// TODO(#30453): See if we can pass this back up the stack so caller can provide this instead of taking up
// space in CommandHanlder.
Expand Down Expand Up @@ -711,7 +727,11 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat
//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mState == State::NewResponseMessage || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);
if (mState == State::AddedCommand)
{
CreateBackupForResponseRollback();
}

auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aCommandPath);
VerifyOrReturnError(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -746,11 +766,26 @@ CHIP_ERROR CommandHandler::FinishStatus()
return CHIP_NO_ERROR;
}

void CommandHandler::CreateBackupForResponseRollback()
{
VerifyOrReturn(mState == State::NewResponseMessage || mState == State::AddedCommand);
VerifyOrReturn(mInvokeResponseBuilder.GetInvokeResponses().GetError() == CHIP_NO_ERROR);
VerifyOrReturn(mInvokeResponseBuilder.GetError() == CHIP_NO_ERROR);
mInvokeResponseBuilder.Checkpoint(mBackupWriter);
mBackupState = mState;
mRollbackBackupValid = true;
}

CHIP_ERROR CommandHandler::RollbackResponse()
{
VerifyOrReturnError(mRollbackBackupValid, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE);
// TODO(#30453): Rollback of mInvokeResponseBuilder should handle resetting
// InvokeResponses.
mInvokeResponseBuilder.GetInvokeResponses().ResetError();
mInvokeResponseBuilder.Rollback(mBackupWriter);
MoveToState(mBackupState);
mRollbackBackupValid = false;
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -801,6 +836,24 @@ CommandHandler::Handle::Handle(CommandHandler * handle)
}
}

CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext()
{
ReturnErrorOnFailure(FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ true));
// After successfully finalizing InvokeResponseMessage, no buffer should remain
// allocated.
VerifyOrDie(!mBufferAllocated);
CHIP_ERROR err = AllocateBuffer();
if (err != CHIP_NO_ERROR)
{
// TODO(#30453): Improve ResponseDropped calls to occur only when dropping is
// definitively guaranteed.
// Response dropping is not yet definitive as a subsequent call
// to AllocateBuffer might succeed.
mResponseSender.ResponseDropped();
}
return err;
}

CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks)
{
System::PacketBufferHandle packet;
Expand All @@ -817,6 +870,8 @@ CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks)
ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage());
ReturnErrorOnFailure(mCommandMessageWriter.Finalize(&packet));
mResponseSender.AddInvokeResponseToSend(std::move(packet));
mBufferAllocated = false;
mRollbackBackupValid = false;
return CHIP_NO_ERROR;
}

Expand All @@ -828,6 +883,9 @@ const char * CommandHandler::GetStateStr() const
case State::Idle:
return "Idle";

case State::NewResponseMessage:
return "NewResponseMessage";

case State::Preparing:
return "Preparing";

Expand Down
154 changes: 115 additions & 39 deletions src/app/CommandHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <lib/support/BitFlags.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/DLLUtil.h>
#include <lib/support/Scoped.h>
#include <lib/support/logging/CHIPLogging.h>
#include <messaging/ExchangeHolder.h>
#include <messaging/Flags.h>
Expand Down Expand Up @@ -288,27 +289,6 @@ class CommandHandler
// This will be completed in a follow up PR.
CHIP_ERROR FinishCommand(bool aEndDataStruct = true);

/**
* This will add a new CommandStatusIB element into InvokeResponses. It will put the
* aCommandPath into the CommandPath element within CommandStatusIB.
*
* This call will fail if CommandHandler is already in the middle of building a
* CommandStatusIB or CommandDataIB (i.e. something has called Prepare*, without
* calling Finish*), or is already sending InvokeResponseMessage.
*
* Upon success, the caller is expected to call `FinishStatus` once they have encoded
* StatusIB.
*
* @param [in] aCommandPath the concrete path of the command we are responding to.
*/
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);

/**
* Finishes the CommandStatusIB element within the InvokeResponses.
*
* Caller must have first successfully called `PrepareStatus`.
*/
CHIP_ERROR FinishStatus();
TLV::TLVWriter * GetCommandDataIBTLVWriter();

/**
Expand All @@ -319,6 +299,54 @@ class CommandHandler
*/
FabricIndex GetAccessingFabricIndex() const;

/**
* @brief Best effort to add InvokeResponse to InvokeResponseMessage.
*
* Tries to add response using lambda. Upon failure to add response, attempts
* to rollback the InvokeResponseMessage to a known good state. If failure is due
* to insufficient space in the current InvokeResponseMessage:
* - Finalizes the current InvokeResponseMessage.
* - Allocates a new InvokeResponseMessage.
* - Reattempts to add the InvokeResponse to the new InvokeResponseMessage.
*
* @param [in] addResponseFunction A lambda function responsible for adding the
* response to the current InvokeResponseMessage.
*/
template <typename Function>
CHIP_ERROR TryAddingResponse(Function && addResponseFunction)
{
// Invalidate any existing rollback backups. The addResponseFunction is
// expected to create a new backup during either PrepareInvokeResponseCommand
// or PrepareStatus execution. Direct invocation of
// CreateBackupForResponseRollback is avoided since the buffer used by
// InvokeResponseMessage might not be allocated until a Prepare* function
// is called.
mRollbackBackupValid = false;
CHIP_ERROR err = addResponseFunction();
if (err == CHIP_NO_ERROR)
{
return CHIP_NO_ERROR;
}
ReturnErrorOnFailure(RollbackResponse());
// If we failed to add a command due to lack of space in the
// packet, we will make another attempt to add the response using
// an additional InvokeResponseMessage.
if (mState != State::AddedCommand || err != CHIP_ERROR_NO_MEMORY)
{
return err;
}
ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext());
err = addResponseFunction();
if (err != CHIP_NO_ERROR)
{
// The return value of RollbackResponse is ignored, as we prioritize
// conveying the error generated by addResponseFunction to the
// caller.
RollbackResponse();
}
return err;
}

/**
* API for adding a data response. The template parameter T is generally
* expected to be a ClusterName::Commands::CommandName::Type struct, but any
Expand All @@ -332,15 +360,24 @@ class CommandHandler
template <typename CommandData>
CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
// TryAddResponseData will ensure we are in the correct state when calling AddResponseData.
CHIP_ERROR err = TryAddResponseData(aRequestCommandPath, aData);
if (err != CHIP_NO_ERROR)
{
// The state guarantees that either we can rollback or we don't have to rollback the buffer, so we don't care about the
// return value of RollbackResponse.
RollbackResponse();
}
return err;
// This method, templated with CommandData, captures all the components needs
// from CommandData with as little code as possible. This in theory should
// reduce compiled code size.
//
// TODO(#30453): Verify the accuracy of the theory outlined below.
//
// Theory on code reduction: Previously, non-essential code was unnecessarily
// templated, leading to compilation and duplication N times. The lambda
// function below mitigates this issue by isolating only the code segments
// that genuinely require templating, thereby minimizing duplicate compiled
// code.
ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
CommandData::GetCommandId() };
auto encodeCommandDataClosure = [&](TLV::TLVWriter & writer) -> CHIP_ERROR {
return DataModel::Encode(writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData);
};
return TryAddingResponse(
[&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, responsePath, encodeCommandDataClosure); });
}

/**
Expand Down Expand Up @@ -416,6 +453,7 @@ class CommandHandler
enum class State : uint8_t
{
Idle, ///< Default state that the object starts out in, where no work has commenced
NewResponseMessage, ///< mInvokeResponseBuilder is ready, with no responses added.
Preparing, ///< We are prepaing the command or status header.
AddingCommand, ///< In the process of adding a command.
AddedCommand, ///< A command has been completely encoded and is awaiting transmission.
Expand All @@ -427,7 +465,14 @@ class CommandHandler
const char * GetStateStr() const;

/**
* Rollback the state to before encoding the current ResponseData (before calling PrepareCommand / PrepareStatus)
* Create a backup to enable rolling back to the state prior to ResponseData encoding in the event of failure.
*/
void CreateBackupForResponseRollback();

/**
* Rollback the state to before encoding the current ResponseData (before calling PrepareInvokeResponseCommand / PrepareStatus)
*
* Requires CreateBackupForResponseRollback to be called at the start of PrepareInvokeResponseCommand / PrepareStatus
*/
CHIP_ERROR RollbackResponse();

Expand Down Expand Up @@ -461,12 +506,34 @@ class CommandHandler
*/
CHIP_ERROR AllocateBuffer();

/**
* This will add a new CommandStatusIB element into InvokeResponses. It will put the
* aCommandPath into the CommandPath element within CommandStatusIB.
*
* This call will fail if CommandHandler is already in the middle of building a
* CommandStatusIB or CommandDataIB (i.e. something has called Prepare*, without
* calling Finish*), or is already sending InvokeResponseMessage.
*
* Upon success, the caller is expected to call `FinishStatus` once they have encoded
* StatusIB.
*
* @param [in] aCommandPath the concrete path of the command we are responding to.
*/
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);

/**
* Finishes the CommandStatusIB element within the InvokeResponses.
*
* Caller must have first successfully called `PrepareStatus`.
*/
CHIP_ERROR FinishStatus();

CHIP_ERROR PrepareInvokeResponseCommand(const CommandPathRegistryEntry & apCommandPathRegistryEntry,
const ConcreteCommandPath & aCommandPath, bool aStartDataStruct);

CHIP_ERROR FinalizeLastInvokeResponseMessage() { return FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ false); }

CHIP_ERROR FinalizeIntermediateInvokeResponseMessage() { return FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ true); }
CHIP_ERROR FinalizeInvokeResponseMessageAndPrepareNext();

CHIP_ERROR FinalizeInvokeResponseMessage(bool aHasMoreChunks);

Expand Down Expand Up @@ -495,6 +562,8 @@ class CommandHandler
Protocols::InteractionModel::Status ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement);
CHIP_ERROR StartSendingCommandResponses();

CHIP_ERROR TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus);

CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus);

/**
Expand All @@ -503,23 +572,25 @@ class CommandHandler
*
* @param [in] aRequestCommandPath the concrete path of the command we are
* responding to.
* @param [in] aData the data for the response.
* @param [in] aResponseCommandPath the concrete command response path.
* @param [in] encodeCommandDataFunction A lambda function responsible for
* encoding the CommandData field.
*/
template <typename CommandData>
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
template <typename Function>
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath,
Function && encodeCommandDataFunction)
{
// Return early in case of requests targeted to a group, since they should not add a response.
VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR);

InvokeResponseParameters prepareParams(aRequestCommandPath);
prepareParams.SetStartOrEndDataStruct(false);

ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
CommandData::GetCommandId() };
ReturnErrorOnFailure(PrepareInvokeResponseCommand(responsePath, prepareParams));
ScopedChange<bool> internalCallToAddResponse(mInternalCallToAddResponseData, true);
ReturnErrorOnFailure(PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData));
ReturnErrorOnFailure(encodeCommandDataFunction(*writer));

return FinishCommand(/* aEndDataStruct = */ false);
}
Expand Down Expand Up @@ -557,12 +628,17 @@ class CommandHandler

State mState = State::Idle;
State mBackupState;
ScopedChangeOnly<bool> mInternalCallToAddResponseData{ false };
bool mSuppressResponse = false;
bool mTimedRequest = false;
bool mSentStatusResponse = false;
bool mGroupRequest = false;
bool mBufferAllocated = false;
bool mReserveSpaceForMoreChunkMessages = false;
// TODO(#30453): We should introduce breaking change where calls to add CommandData
// need to use AddResponse, and not CommandHandler primitives directly using
// GetCommandDataIBTLVWriter.
bool mRollbackBackupValid = false;
// If mGoneAsync is true, we have finished out initial processing of the
// incoming invoke. After this point, our session could go away at any
// time.
Expand Down
Loading

0 comments on commit 4431191

Please sign in to comment.