Skip to content

Commit 1cbcc5c

Browse files
Rollback InvokeRequestMessage when AddResponseData fails (#33849)
* Rollback InvokeRequestMessage when AddResponseData fails * Restyled by clang-format * Fix rebase issues with TestCommandInteraction * Move new test to pigweed style unit test after rebase * Address PR comments * Restyled by clang-format * Address PR comments * Address PR comments * Small fixes * Small fixes * Restyled by clang-format * Address PR comment --------- Co-authored-by: Restyled.io <commits@restyled.io>
1 parent dbd01cc commit 1cbcc5c

File tree

3 files changed

+125
-9
lines changed

3 files changed

+125
-9
lines changed

src/app/CommandSender.cpp

+35-1
Original file line numberDiff line numberDiff line change
@@ -555,13 +555,18 @@ CHIP_ERROR CommandSender::FinishCommand(FinishCommandParameters & aFinishCommand
555555
CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, const DataModel::EncodableToTLV & aEncodable,
556556
AddRequestDataParameters & aAddRequestDataParams)
557557
{
558+
ReturnErrorOnFailure(AllocateBuffer());
559+
560+
RollbackInvokeRequest rollback(*this);
558561
PrepareCommandParameters prepareCommandParams(aAddRequestDataParams);
559562
ReturnErrorOnFailure(PrepareCommand(aCommandPath, prepareCommandParams));
560563
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
561564
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
562565
ReturnErrorOnFailure(aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields)));
563566
FinishCommandParameters finishCommandParams(aAddRequestDataParams);
564-
return FinishCommand(finishCommandParams);
567+
ReturnErrorOnFailure(FinishCommand(finishCommandParams));
568+
rollback.DisableAutomaticRollback();
569+
return CHIP_NO_ERROR;
565570
}
566571

567572
CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinishCommandParams)
@@ -672,5 +677,34 @@ void CommandSender::MoveToState(const State aTargetState)
672677
ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr());
673678
}
674679

680+
CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender & aCommandSender) : mCommandSender(aCommandSender)
681+
{
682+
VerifyOrReturn(mCommandSender.mBufferAllocated);
683+
VerifyOrReturn(mCommandSender.mState == State::Idle || mCommandSender.mState == State::AddedCommand);
684+
VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR);
685+
VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR);
686+
mCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter);
687+
mBackupState = mCommandSender.mState;
688+
mRollbackInDestructor = true;
689+
}
690+
691+
CommandSender::RollbackInvokeRequest::~RollbackInvokeRequest()
692+
{
693+
VerifyOrReturn(mRollbackInDestructor);
694+
VerifyOrReturn(mCommandSender.mState == State::AddingCommand);
695+
ChipLogDetail(DataManagement, "Rolling back response");
696+
// TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting
697+
// InvokeRequests.
698+
mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError();
699+
mCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter);
700+
mCommandSender.MoveToState(mBackupState);
701+
mRollbackInDestructor = false;
702+
}
703+
704+
void CommandSender::RollbackInvokeRequest::DisableAutomaticRollback()
705+
{
706+
mRollbackInDestructor = false;
707+
}
708+
675709
} // namespace app
676710
} // namespace chip

src/app/CommandSender.h

+34
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,12 @@ class CommandSender final : public Messaging::ExchangeDelegate
216216

217217
AddRequestDataParameters(const Optional<uint16_t> & aTimedInvokeTimeoutMs) : timedInvokeTimeoutMs(aTimedInvokeTimeoutMs) {}
218218

219+
AddRequestDataParameters & SetCommandRef(uint16_t aCommandRef)
220+
{
221+
commandRef.SetValue(aCommandRef);
222+
return *this;
223+
}
224+
219225
// When a value is provided for timedInvokeTimeoutMs, this invoke becomes a timed
220226
// invoke. CommandSender will use the minimum of all provided timeouts for execution.
221227
const Optional<uint16_t> timedInvokeTimeoutMs;
@@ -512,6 +518,34 @@ class CommandSender final : public Messaging::ExchangeDelegate
512518
AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application.
513519
};
514520

521+
/**
522+
* Class to help backup CommandSender's buffer containing InvokeRequestMessage when adding InvokeRequest
523+
* in case there is a failure to add InvokeRequest. Intended usage is as follows:
524+
* - Allocate RollbackInvokeRequest on the stack.
525+
* - Attempt adding InvokeRequest into InvokeRequestMessage buffer.
526+
* - If modification is added successfully, call DisableAutomaticRollback() to prevent destructor from
527+
* rolling back InvokeReqestMessage.
528+
* - If there is an issue adding InvokeRequest, destructor will take care of rolling back
529+
* InvokeRequestMessage to previously saved state.
530+
*/
531+
class RollbackInvokeRequest
532+
{
533+
public:
534+
explicit RollbackInvokeRequest(CommandSender & aCommandSender);
535+
~RollbackInvokeRequest();
536+
537+
/**
538+
* Disables rolling back to previously saved state for InvokeRequestMessage.
539+
*/
540+
void DisableAutomaticRollback();
541+
542+
private:
543+
CommandSender & mCommandSender;
544+
TLV::TLVWriter mBackupWriter;
545+
State mBackupState;
546+
bool mRollbackInDestructor = false;
547+
};
548+
515549
union CallbackHandle
516550
{
517551
CallbackHandle(Callback * apCallback) : legacyCallback(apCallback) {}

src/app/tests/TestCommandInteraction.cpp

+56-8
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,9 @@ enum class ForcedSizeBufferLengthHint
109109
kSizeGreaterThan255,
110110
};
111111

112-
struct ForcedSizeBuffer
112+
class ForcedSizeBuffer : public app::DataModel::EncodableToTLV
113113
{
114-
chip::Platform::ScopedMemoryBufferWithSize<uint8_t> mBuffer;
115-
114+
public:
116115
ForcedSizeBuffer(uint32_t size)
117116
{
118117
if (mBuffer.Alloc(size))
@@ -124,7 +123,7 @@ struct ForcedSizeBuffer
124123

125124
// No significance with using 0x12 as the CommandId, just using a value.
126125
static constexpr chip::CommandId GetCommandId() { return 0x12; }
127-
CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const
126+
CHIP_ERROR EncodeTo(TLV::TLVWriter & aWriter, TLV::Tag aTag) const override
128127
{
129128
VerifyOrReturnError(mBuffer, CHIP_ERROR_NO_MEMORY);
130129

@@ -133,6 +132,9 @@ struct ForcedSizeBuffer
133132
ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize())));
134133
return aWriter.EndContainer(outerContainerType);
135134
}
135+
136+
private:
137+
chip::Platform::ScopedMemoryBufferWithSize<uint8_t> mBuffer;
136138
};
137139

138140
struct Fields
@@ -387,6 +389,7 @@ class TestCommandInteraction : public ::testing::Test
387389
void TestCommandSender_WithProcessReceivedMsg();
388390
void TestCommandSender_ExtendableApiWithProcessReceivedMsg();
389391
void TestCommandSender_ExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef();
392+
void TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked();
390393
void TestCommandHandler_WithoutResponderCallingAddStatus();
391394
void TestCommandHandler_WithoutResponderCallingAddResponse();
392395
void TestCommandHandler_WithoutResponderCallingDirectPrepareFinishCommandApis();
@@ -632,7 +635,8 @@ uint32_t TestCommandInteraction::GetAddResponseDataOverheadSizeForPath(const Con
632635
// When ForcedSizeBuffer exceeds 255, an extra byte is needed for length, affecting the overhead size required by
633636
// AddResponseData. In order to have this accounted for in overhead calculation we set the length to be 256.
634637
uint32_t sizeOfForcedSizeBuffer = aBufferSizeHint == ForcedSizeBufferLengthHint::kSizeGreaterThan255 ? 256 : 0;
635-
EXPECT_EQ(commandHandler.AddResponseData(aRequestCommandPath, ForcedSizeBuffer(sizeOfForcedSizeBuffer)), CHIP_NO_ERROR);
638+
ForcedSizeBuffer responseData(sizeOfForcedSizeBuffer);
639+
EXPECT_EQ(commandHandler.AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR);
636640
uint32_t remainingSizeAfter = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength();
637641
uint32_t delta = remainingSizeBefore - remainingSizeAfter - sizeOfForcedSizeBuffer;
638642

@@ -657,7 +661,8 @@ void TestCommandInteraction::FillCurrentInvokeResponseBuffer(CommandHandlerImpl
657661
// Validating assumption. If this fails, it means overheadSizeNeededForAddingResponse is likely too large.
658662
EXPECT_GE(sizeToFill, 256u);
659663

660-
EXPECT_EQ(apCommandHandler->AddResponseData(aRequestCommandPath, ForcedSizeBuffer(sizeToFill)), CHIP_NO_ERROR);
664+
ForcedSizeBuffer responseData(sizeToFill);
665+
EXPECT_EQ(apCommandHandler->AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR);
661666
}
662667

663668
void TestCommandInteraction::ValidateCommandHandlerEncodeInvokeResponseMessage(bool aNeedStatusCode)
@@ -1087,6 +1092,47 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandSender_ExtendableApiWithP
10871092
EXPECT_EQ(mockCommandSenderExtendedDelegate.onErrorCalledTimes, 0);
10881093
}
10891094

1095+
TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked)
1096+
{
1097+
mockCommandSenderExtendedDelegate.ResetCounter();
1098+
PendingResponseTrackerImpl pendingResponseTracker;
1099+
app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate,
1100+
&mpTestContext->GetExchangeManager(), &pendingResponseTracker);
1101+
1102+
app::CommandSender::AddRequestDataParameters addRequestDataParams;
1103+
1104+
CommandSender::ConfigParameters config;
1105+
config.SetRemoteMaxPathsPerInvoke(2);
1106+
EXPECT_EQ(commandSender.SetCommandSenderConfig(config), CHIP_NO_ERROR);
1107+
1108+
// The specific values chosen here are arbitrary.
1109+
uint16_t firstCommandRef = 1;
1110+
uint16_t secondCommandRef = 2;
1111+
auto commandPathParams = MakeTestCommandPath();
1112+
SimpleTLVPayload simplePayloadWriter;
1113+
addRequestDataParams.SetCommandRef(firstCommandRef);
1114+
1115+
EXPECT_EQ(commandSender.AddRequestData(commandPathParams, simplePayloadWriter, addRequestDataParams), CHIP_NO_ERROR);
1116+
1117+
uint32_t remainingSize = commandSender.mInvokeRequestBuilder.GetWriter()->GetRemainingFreeLength();
1118+
// Because request is made of both request data and request path (commandPathParams), using
1119+
// `remainingSize` is large enough fail.
1120+
ForcedSizeBuffer requestData(remainingSize);
1121+
1122+
addRequestDataParams.SetCommandRef(secondCommandRef);
1123+
EXPECT_EQ(commandSender.AddRequestData(commandPathParams, requestData, addRequestDataParams), CHIP_ERROR_NO_MEMORY);
1124+
1125+
// Confirm that we can still send out a request with the first command.
1126+
EXPECT_EQ(commandSender.SendCommandRequest(mpTestContext->GetSessionBobToAlice()), CHIP_NO_ERROR);
1127+
EXPECT_EQ(commandSender.GetInvokeResponseMessageCount(), 0u);
1128+
1129+
mpTestContext->DrainAndServiceIO();
1130+
1131+
EXPECT_EQ(mockCommandSenderExtendedDelegate.onResponseCalledTimes, 1);
1132+
EXPECT_EQ(mockCommandSenderExtendedDelegate.onFinalCalledTimes, 1);
1133+
EXPECT_EQ(mockCommandSenderExtendedDelegate.onErrorCalledTimes, 0);
1134+
}
1135+
10901136
TEST_F(TestCommandInteraction, TestCommandHandlerEncodeSimpleCommandData)
10911137
{
10921138
// Send response which has simple command data and command path
@@ -1188,7 +1234,8 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_WithoutResponderC
11881234
CommandHandlerImpl commandHandler(&mockCommandHandlerDelegate);
11891235

11901236
uint32_t sizeToFill = 50; // This is an arbitrary number, we need to select a non-zero value.
1191-
EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath, ForcedSizeBuffer(sizeToFill)), CHIP_NO_ERROR);
1237+
ForcedSizeBuffer responseData(sizeToFill);
1238+
EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR);
11921239

11931240
// Since calling AddResponseData is supposed to be a no-operation when there is no responder, it is
11941241
// hard to validate. Best way is to check that we are still in an Idle state afterwards
@@ -1813,7 +1860,8 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_FillUpInvokeRespo
18131860
EXPECT_EQ(remainingSize, sizeToLeave);
18141861

18151862
uint32_t sizeToFill = 50;
1816-
EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath2, ForcedSizeBuffer(sizeToFill)), CHIP_NO_ERROR);
1863+
ForcedSizeBuffer responseData(sizeToFill);
1864+
EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath2, responseData.GetCommandId(), responseData), CHIP_NO_ERROR);
18171865

18181866
remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength();
18191867
EXPECT_GT(remainingSize, sizeToLeave);

0 commit comments

Comments
 (0)