From aac2b86b3d53aae214e002f491a508ca0fe6f07b Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 7 Jun 2024 18:54:15 +0000 Subject: [PATCH 01/12] Rollback InvokeRequestMessage when AddResponseData fails --- src/app/CommandSender.cpp | 49 +++++++++++++-- src/app/CommandSender.h | 32 ++++++++++ src/app/tests/TestCommandInteraction.cpp | 80 ++++++++++++++++++++---- 3 files changed, 143 insertions(+), 18 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index fe263eb8923cc7..ebe54af3d4eecc 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -128,6 +128,30 @@ CHIP_ERROR CommandSender::SendCommandRequestInternal(const SessionHandle & sessi return SendInvokeRequest(); } +void CommandSender::CreateBackupForRequestRollback(RollbackData & aRollbackData) +{ + VerifyOrReturn(mBufferAllocated); + VerifyOrReturn(mState == State::Idle || mState == State::AddedCommand); + VerifyOrReturn(mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); + VerifyOrReturn(mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); + mInvokeRequestBuilder.Checkpoint(aRollbackData.backupWriter); + aRollbackData.backupState = mState; + aRollbackData.rollbackIsValid = true; +} + +void CommandSender::RollbackRequest(RollbackData & aRollbackData) +{ + VerifyOrReturn(aRollbackData.rollbackIsValid); + VerifyOrReturn(mState == State::AddingCommand); + ChipLogDetail(DataManagement, "Rolling back response"); + // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting + // InvokeResponses. + mInvokeRequestBuilder.GetInvokeRequests().ResetError(); + mInvokeRequestBuilder.Rollback(aRollbackData.backupWriter); + MoveToState(aRollbackData.backupState); + aRollbackData.rollbackIsValid = false; +} + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST CHIP_ERROR CommandSender::TestOnlyCommandSenderTimedRequestFlagWithNoTimedInvoke(const SessionHandle & session, Optional timeout) @@ -540,13 +564,26 @@ CHIP_ERROR CommandSender::FinishCommand(FinishCommandParameters & aFinishCommand CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, const DataModel::EncodableToTLV & aEncodable, AddRequestDataParameters & aAddRequestDataParams) { + ReturnErrorOnFailure(AllocateBuffer()); + + RollbackData rollbackData; + CreateBackupForRequestRollback(rollbackData); PrepareCommandParameters prepareCommandParams(aAddRequestDataParams); - ReturnErrorOnFailure(PrepareCommand(aCommandPath, prepareCommandParams)); - TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); - VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields))); - FinishCommandParameters finishCommandParams(aAddRequestDataParams); - return FinishCommand(finishCommandParams); + TLV::TLVWriter * writer = nullptr; + CHIP_ERROR err = CHIP_NO_ERROR; + SuccessOrExit(err = PrepareCommand(aCommandPath, prepareCommandParams)); + writer = GetCommandDataIBTLVWriter(); + VerifyOrExit(writer != nullptr, err = CHIP_ERROR_INCORRECT_STATE); + SuccessOrExit(err = aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields))); + { + FinishCommandParameters finishCommandParams(aAddRequestDataParams); + SuccessOrExit(err = FinishCommand(finishCommandParams)); + } +exit: + if (err != CHIP_NO_ERROR) { + RollbackRequest(rollbackData); + } + return err; } CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinishCommandParams) diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 5542d2469bb743..fc6ad0e00e8015 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -216,6 +216,12 @@ class CommandSender final : public Messaging::ExchangeDelegate AddRequestDataParameters(const Optional & aTimedInvokeTimeoutMs) : timedInvokeTimeoutMs(aTimedInvokeTimeoutMs) {} + AddRequestDataParameters & SetCommandRef(uint16_t aCommandRef) + { + commandRef.SetValue(aCommandRef); + return *this; + } + // When a value is provided for timedInvokeTimeoutMs, this invoke becomes a timed // invoke. CommandSender will use the minimum of all provided timeouts for execution. const Optional timedInvokeTimeoutMs; @@ -511,6 +517,12 @@ class CommandSender final : public Messaging::ExchangeDelegate AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application. }; + struct RollbackData { + TLV::TLVWriter backupWriter; + State backupState; + bool rollbackIsValid = false; + }; + union CallbackHandle { CallbackHandle(Callback * apCallback) : legacyCallback(apCallback) {} @@ -566,6 +578,26 @@ class CommandSender final : public Messaging::ExchangeDelegate CHIP_ERROR SendCommandRequestInternal(const SessionHandle & session, Optional timeout); + /** + * Creates a backup to enable rolling back buffer containing InvokeRequestMessage + * in case subsequent calls to add request fail. + * + * A successful backup will only be created if the InvokeRequestMessage is + * in a known good state. + * + * @param [in] aRollbackData reference to rollback data that can be on the stack. + */ + void CreateBackupForRequestRollback(RollbackData & aRollbackData); + + /** + * Rolls back buffer containing InvokeRequestMessage to a previously saved state. + * + * @param [in] aRollbackData reference to rollback data that was previously provided + * to CreateBackupForRequestRollback. This rollbackData must be valid for + * a successful rollback. + */ + void RollbackRequest(RollbackData & aRollbackData); + void OnResponseCallback(const ResponseData & aResponseData) { // mpExtendableCallback and mpCallback are mutually exclusive. diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 2a5791ba839423..1c436cbe70cdd0 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -107,30 +107,32 @@ namespace app { CommandHandler::Handle asyncCommandHandle; -struct ForcedSizeBuffer +class ForcedSizeBuffer : public app::DataModel::EncodableToTLV { - chip::Platform::ScopedMemoryBufferWithSize mBuffer; - +public: ForcedSizeBuffer(uint32_t size) { - if (mBuffer.Alloc(size)) + if (mBuffer_.Alloc(size)) { // No significance with using 0x12, just using a value. - memset(mBuffer.Get(), 0x12, size); + memset(mBuffer_.Get(), 0x12, size); } } // No significance with using 0x12 as the CommandId, just using a value. static constexpr chip::CommandId GetCommandId() { return 0x12; } - CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const + CHIP_ERROR EncodeTo(TLV::TLVWriter & aWriter, TLV::Tag aTag) const override { - VerifyOrReturnError(mBuffer, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(mBuffer_, CHIP_ERROR_NO_MEMORY); TLV::TLVType outerContainerType; ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); - ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize()))); + ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer_.Get(), mBuffer_.AllocatedSize()))); return aWriter.EndContainer(outerContainerType); } + +private: + chip::Platform::ScopedMemoryBufferWithSize mBuffer_; }; enum class ForcedSizeBufferLengthHint @@ -361,6 +363,8 @@ class TestCommandInteraction static void TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext); + static void TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked(nlTestSuite * apSuite, + void * apContext); static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse(nlTestSuite * apSuite, void * apContext); static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative(nlTestSuite * apSuite, @@ -649,7 +653,8 @@ uint32_t TestCommandInteraction::GetAddResponseDataOverheadSizeForPath(nlTestSui // When ForcedSizeBuffer exceeds 255, an extra byte is needed for length, affecting the overhead size required by // AddResponseData. In order to have this accounted for in overhead calculation we set the length to be 256. uint32_t sizeOfForcedSizeBuffer = aBufferSizeHint == ForcedSizeBufferLengthHint::kSizeGreaterThan255 ? 256 : 0; - err = commandHandler.AddResponseData(aRequestCommandPath, ForcedSizeBuffer(sizeOfForcedSizeBuffer)); + ForcedSizeBuffer responseData(sizeOfForcedSizeBuffer); + err = commandHandler.AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); uint32_t remainingSizeAfter = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); uint32_t delta = remainingSizeBefore - remainingSizeAfter - sizeOfForcedSizeBuffer; @@ -679,7 +684,8 @@ void TestCommandInteraction::FillCurrentInvokeResponseBuffer(nlTestSuite * apSui // Validating assumption. If this fails, it means overheadSizeNeededForAddingResponse is likely too large. NL_TEST_ASSERT(apSuite, sizeToFill >= 256); - err = apCommandHandler->AddResponseData(aRequestCommandPath, ForcedSizeBuffer(sizeToFill)); + ForcedSizeBuffer responseData(sizeToFill); + err = apCommandHandler->AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } @@ -1314,7 +1320,8 @@ void TestCommandInteraction::TestCommandHandlerWithoutResponderCallingAddRespons CommandHandlerImpl commandHandler(&mockCommandHandlerDelegate); uint32_t sizeToFill = 50; // This is an arbitrary number, we need to select a non-zero value. - CHIP_ERROR err = commandHandler.AddResponseData(requestCommandPath, ForcedSizeBuffer(sizeToFill)); + ForcedSizeBuffer responseData(sizeToFill); + CHIP_ERROR err = commandHandler.AddResponseData(requestCommandPath, responseData.GetCommandId(), responseData); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); // Since calling AddResponseData is supposed to be a no-operation when there is no responder, it is @@ -1940,6 +1947,53 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit NL_TEST_ASSERT(apSuite, commandDispatchedCount == 2); } +void TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked( + nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + mockCommandSenderExtendedDelegate.ResetCounter(); + PendingResponseTrackerImpl pendingResponseTracker; + app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(), + &pendingResponseTracker); + app::CommandSender::AddRequestDataParameters addRequestDataParams; + + CommandSender::ConfigParameters config; + config.SetRemoteMaxPathsPerInvoke(2); + err = commandSender.SetCommandSenderConfig(config); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // The specific values chosen here are arbitrary. + uint16_t firstCommandRef = 1; + uint16_t secondCommandRef = 2; + auto commandPathParams = MakeTestCommandPath(); + SimpleTLVPayload simplePayloadWriter; + addRequestDataParams.SetCommandRef(firstCommandRef); + + err = commandSender.AddRequestData(commandPathParams, simplePayloadWriter, addRequestDataParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + uint32_t remainingSize = commandSender.mInvokeRequestBuilder.GetWriter()->GetRemainingFreeLength(); + // Because request is made of both request data and request path (commandPathParams), using + // `remainingSize` is large enough fail. + ForcedSizeBuffer requestData(remainingSize); + + addRequestDataParams.SetCommandRef(secondCommandRef); + err = commandSender.AddRequestData(commandPathParams, requestData, addRequestDataParams); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NO_MEMORY); + + // Confirm that we can still send out a request with the first command. + err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, commandSender.GetInvokeResponseMessageCount() == 0); + + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, + mockCommandSenderExtendedDelegate.onResponseCalledTimes == 1 && mockCommandSenderExtendedDelegate.onFinalCalledTimes == 1 && + mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0); +} + void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse( nlTestSuite * apSuite, void * apContext) { @@ -2020,7 +2074,8 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere NL_TEST_ASSERT(apSuite, remainingSize == sizeToLeave); uint32_t sizeToFill = 50; - err = commandHandler.AddResponseData(requestCommandPath2, ForcedSizeBuffer(sizeToFill)); + ForcedSizeBuffer responseData(sizeToFill); + err = commandHandler.AddResponseData(requestCommandPath2, responseData.GetCommandId(), responseData); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); @@ -2093,6 +2148,7 @@ const nlTest sTests[] = #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne), NL_TEST_DEF("TestCommandHandlerAcceptMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands), + NL_TEST_DEF("TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked", chip::app::TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked), NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse), NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative), NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse), From d51fae5b3ef540ed11faa587814be3a176db9083 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 11 Jun 2024 17:21:24 +0000 Subject: [PATCH 02/12] Restyled by clang-format --- src/app/CommandSender.cpp | 7 ++++--- src/app/CommandSender.h | 3 ++- src/app/tests/TestCommandInteraction.cpp | 14 +++++++------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index ebe54af3d4eecc..3f361fcc25b5b5 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -135,7 +135,7 @@ void CommandSender::CreateBackupForRequestRollback(RollbackData & aRollbackData) VerifyOrReturn(mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); VerifyOrReturn(mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); mInvokeRequestBuilder.Checkpoint(aRollbackData.backupWriter); - aRollbackData.backupState = mState; + aRollbackData.backupState = mState; aRollbackData.rollbackIsValid = true; } @@ -570,7 +570,7 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, CreateBackupForRequestRollback(rollbackData); PrepareCommandParameters prepareCommandParams(aAddRequestDataParams); TLV::TLVWriter * writer = nullptr; - CHIP_ERROR err = CHIP_NO_ERROR; + CHIP_ERROR err = CHIP_NO_ERROR; SuccessOrExit(err = PrepareCommand(aCommandPath, prepareCommandParams)); writer = GetCommandDataIBTLVWriter(); VerifyOrExit(writer != nullptr, err = CHIP_ERROR_INCORRECT_STATE); @@ -580,7 +580,8 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, SuccessOrExit(err = FinishCommand(finishCommandParams)); } exit: - if (err != CHIP_NO_ERROR) { + if (err != CHIP_NO_ERROR) + { RollbackRequest(rollbackData); } return err; diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index fc6ad0e00e8015..a99cf47d52cb30 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -517,7 +517,8 @@ class CommandSender final : public Messaging::ExchangeDelegate AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application. }; - struct RollbackData { + struct RollbackData + { TLV::TLVWriter backupWriter; State backupState; bool rollbackIsValid = false; diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 1c436cbe70cdd0..f6a143bbbb8951 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -127,7 +127,8 @@ class ForcedSizeBuffer : public app::DataModel::EncodableToTLV TLV::TLVType outerContainerType; ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); - ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer_.Get(), mBuffer_.AllocatedSize()))); + ReturnErrorOnFailure( + app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer_.Get(), mBuffer_.AllocatedSize()))); return aWriter.EndContainer(outerContainerType); } @@ -363,8 +364,7 @@ class TestCommandInteraction static void TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext); - static void TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked(nlTestSuite * apSuite, - void * apContext); + static void TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked(nlTestSuite * apSuite, void * apContext); static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse(nlTestSuite * apSuite, void * apContext); static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative(nlTestSuite * apSuite, @@ -1947,8 +1947,7 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit NL_TEST_ASSERT(apSuite, commandDispatchedCount == 2); } -void TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked( - nlTestSuite * apSuite, void * apContext) +void TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; @@ -1966,7 +1965,7 @@ void TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestData // The specific values chosen here are arbitrary. uint16_t firstCommandRef = 1; uint16_t secondCommandRef = 2; - auto commandPathParams = MakeTestCommandPath(); + auto commandPathParams = MakeTestCommandPath(); SimpleTLVPayload simplePayloadWriter; addRequestDataParams.SetCommandRef(firstCommandRef); @@ -1990,7 +1989,8 @@ void TestCommandInteraction::TestCommandSender_ValidateSecondLargeAddRequestData ctx.DrainAndServiceIO(); NL_TEST_ASSERT(apSuite, - mockCommandSenderExtendedDelegate.onResponseCalledTimes == 1 && mockCommandSenderExtendedDelegate.onFinalCalledTimes == 1 && + mockCommandSenderExtendedDelegate.onResponseCalledTimes == 1 && + mockCommandSenderExtendedDelegate.onFinalCalledTimes == 1 && mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0); } From f78585334d5100917119bd396ec076d303484a1b Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Jun 2024 13:21:51 +0000 Subject: [PATCH 03/12] Fix rebase issues with TestCommandInteraction --- src/app/tests/TestCommandInteraction.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 95d3c203bc5854..3cd60458399aeb 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -635,7 +635,7 @@ uint32_t TestCommandInteraction::GetAddResponseDataOverheadSizeForPath(const Con // AddResponseData. In order to have this accounted for in overhead calculation we set the length to be 256. uint32_t sizeOfForcedSizeBuffer = aBufferSizeHint == ForcedSizeBufferLengthHint::kSizeGreaterThan255 ? 256 : 0; ForcedSizeBuffer responseData(sizeOfForcedSizeBuffer); - EXPECT_EQ(commandHandler.AddResponseData(aRequestCommandPath, responseData, CHIP_NO_ERROR); + EXPECT_EQ(commandHandler.AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR); uint32_t remainingSizeAfter = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); uint32_t delta = remainingSizeBefore - remainingSizeAfter - sizeOfForcedSizeBuffer; @@ -661,7 +661,7 @@ void TestCommandInteraction::FillCurrentInvokeResponseBuffer(CommandHandlerImpl EXPECT_GE(sizeToFill, 256u); ForcedSizeBuffer responseData(sizeToFill); - EXPECT_EQ(apCommandHandler->AddResponseData(aRequestCommandPath, responseData), CHIP_NO_ERROR); + EXPECT_EQ(apCommandHandler->AddResponseData(aRequestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR); } void TestCommandInteraction::ValidateCommandHandlerEncodeInvokeResponseMessage(bool aNeedStatusCode) @@ -1193,7 +1193,7 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_WithoutResponderC uint32_t sizeToFill = 50; // This is an arbitrary number, we need to select a non-zero value. ForcedSizeBuffer responseData(sizeToFill); - EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath, responseData, CHIP_NO_ERROR); + EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath, responseData.GetCommandId(), responseData), CHIP_NO_ERROR); // Since calling AddResponseData is supposed to be a no-operation when there is no responder, it is // hard to validate. Best way is to check that we are still in an Idle state afterwards @@ -1868,7 +1868,7 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_FillUpInvokeRespo uint32_t sizeToFill = 50; ForcedSizeBuffer responseData(sizeToFill); - EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath2, responseData, CHIP_NO_ERROR); + EXPECT_EQ(commandHandler.AddResponseData(requestCommandPath2, responseData.GetCommandId(), responseData), CHIP_NO_ERROR); remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); EXPECT_GT(remainingSize, sizeToLeave); From a8535fd95acf2c10148f38382b95775946b8b2b2 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Jun 2024 13:33:14 +0000 Subject: [PATCH 04/12] Move new test to pigweed style unit test after rebase --- src/app/tests/TestCommandInteraction.cpp | 90 +++++++++++------------- 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 3cd60458399aeb..5403788385e532 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -1091,6 +1091,47 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandSender_ExtendableApiWithP EXPECT_EQ(mockCommandSenderExtendedDelegate.onErrorCalledTimes, 0); } +TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked) +{ + mockCommandSenderExtendedDelegate.ResetCounter(); + PendingResponseTrackerImpl pendingResponseTracker; + app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, + &mpTestContext->GetExchangeManager(), &pendingResponseTracker); + + app::CommandSender::AddRequestDataParameters addRequestDataParams; + + CommandSender::ConfigParameters config; + config.SetRemoteMaxPathsPerInvoke(2); + EXPECT_EQ(commandSender.SetCommandSenderConfig(config), CHIP_NO_ERROR); + + // The specific values chosen here are arbitrary. + uint16_t firstCommandRef = 1; + uint16_t secondCommandRef = 2; + auto commandPathParams = MakeTestCommandPath(); + SimpleTLVPayload simplePayloadWriter; + addRequestDataParams.SetCommandRef(firstCommandRef); + + EXPECT_EQ(commandSender.AddRequestData(commandPathParams, simplePayloadWriter, addRequestDataParams), CHIP_NO_ERROR); + + uint32_t remainingSize = commandSender.mInvokeRequestBuilder.GetWriter()->GetRemainingFreeLength(); + // Because request is made of both request data and request path (commandPathParams), using + // `remainingSize` is large enough fail. + ForcedSizeBuffer requestData(remainingSize); + + addRequestDataParams.SetCommandRef(secondCommandRef); + EXPECT_EQ(commandSender.AddRequestData(commandPathParams, requestData, addRequestDataParams), CHIP_ERROR_NO_MEMORY); + + // Confirm that we can still send out a request with the first command. + EXPECT_EQ(commandSender.SendCommandRequest(mpTestContext->GetSessionBobToAlice()), CHIP_NO_ERROR); + EXPECT_EQ(commandSender.GetInvokeResponseMessageCount(), 0u); + + mpTestContext->DrainAndServiceIO(); + + EXPECT_EQ(mockCommandSenderExtendedDelegate.onResponseCalledTimes, 1); + EXPECT_EQ(mockCommandSenderExtendedDelegate.onFinalCalledTimes, 1); + EXPECT_EQ(mockCommandSenderExtendedDelegate.onErrorCalledTimes, 0); +} + TEST_F(TestCommandInteraction, TestCommandHandlerEncodeSimpleCommandData) { // Send response which has simple command data and command path @@ -1741,55 +1782,6 @@ TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_AcceptMultipleCom EXPECT_EQ(commandDispatchedCount, 2u); } -#if 0 -TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandSender_ValidateSecondLargeAddRequestDataRollbacked) -{ - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; - mockCommandSenderExtendedDelegate.ResetCounter(); - PendingResponseTrackerImpl pendingResponseTracker; - app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(), - &pendingResponseTracker); - app::CommandSender::AddRequestDataParameters addRequestDataParams; - - CommandSender::ConfigParameters config; - config.SetRemoteMaxPathsPerInvoke(2); - err = commandSender.SetCommandSenderConfig(config); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - // The specific values chosen here are arbitrary. - uint16_t firstCommandRef = 1; - uint16_t secondCommandRef = 2; - auto commandPathParams = MakeTestCommandPath(); - SimpleTLVPayload simplePayloadWriter; - addRequestDataParams.SetCommandRef(firstCommandRef); - - err = commandSender.AddRequestData(commandPathParams, simplePayloadWriter, addRequestDataParams); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - uint32_t remainingSize = commandSender.mInvokeRequestBuilder.GetWriter()->GetRemainingFreeLength(); - // Because request is made of both request data and request path (commandPathParams), using - // `remainingSize` is large enough fail. - ForcedSizeBuffer requestData(remainingSize); - - addRequestDataParams.SetCommandRef(secondCommandRef); - err = commandSender.AddRequestData(commandPathParams, requestData, addRequestDataParams); - NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NO_MEMORY); - - // Confirm that we can still send out a request with the first command. - err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice()); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(apSuite, commandSender.GetInvokeResponseMessageCount() == 0); - - ctx.DrainAndServiceIO(); - - NL_TEST_ASSERT(apSuite, - mockCommandSenderExtendedDelegate.onResponseCalledTimes == 1 && - mockCommandSenderExtendedDelegate.onFinalCalledTimes == 1 && - mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0); -} -#endif - TEST_F_FROM_FIXTURE(TestCommandInteraction, TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse) { BasicCommandPathRegistry<4> basicCommandPathRegistry; From 67667d35a925435cfad13994996994609c2bfff2 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Jun 2024 15:23:49 +0000 Subject: [PATCH 05/12] Address PR comments --- src/app/CommandSender.cpp | 31 ++++++++++++++++-------- src/app/CommandSender.h | 13 +++++++--- src/app/tests/TestCommandInteraction.cpp | 10 ++++---- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 3f361fcc25b5b5..03bb7682964c2d 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -134,22 +134,15 @@ void CommandSender::CreateBackupForRequestRollback(RollbackData & aRollbackData) VerifyOrReturn(mState == State::Idle || mState == State::AddedCommand); VerifyOrReturn(mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); VerifyOrReturn(mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); - mInvokeRequestBuilder.Checkpoint(aRollbackData.backupWriter); - aRollbackData.backupState = mState; - aRollbackData.rollbackIsValid = true; + aRollbackData.Checkpoint(*this); } void CommandSender::RollbackRequest(RollbackData & aRollbackData) { - VerifyOrReturn(aRollbackData.rollbackIsValid); + VerifyOrReturn(aRollbackData.RollbackIsValid()); VerifyOrReturn(mState == State::AddingCommand); ChipLogDetail(DataManagement, "Rolling back response"); - // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting - // InvokeResponses. - mInvokeRequestBuilder.GetInvokeRequests().ResetError(); - mInvokeRequestBuilder.Rollback(aRollbackData.backupWriter); - MoveToState(aRollbackData.backupState); - aRollbackData.rollbackIsValid = false; + LogErrorOnFailure(aRollbackData.Rollback(*this)); } #if CONFIG_BUILD_FOR_HOST_UNIT_TEST @@ -695,5 +688,23 @@ void CommandSender::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr()); } +void CommandSender::RollbackData::Checkpoint(CommandSender& aCommandSender) { + //InvokeRequestMessage::Builder& aInvokeRequestBuilder, const State& aState) { + aCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); + mBackupState = aCommandSender.mState; + mRollbackIsValid = true; +} + +CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender& aCommandSender) { + // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting + // InvokeResponses. + VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE); + aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError(); + aCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter); + aCommandSender.MoveToState(mBackupState); + mRollbackIsValid = false; + return CHIP_NO_ERROR; +} + } // namespace app } // namespace chip diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index a99cf47d52cb30..92fd054cb13357 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -517,11 +517,16 @@ class CommandSender final : public Messaging::ExchangeDelegate AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application. }; - struct RollbackData + class RollbackData { - TLV::TLVWriter backupWriter; - State backupState; - bool rollbackIsValid = false; + public: + void Checkpoint(CommandSender& aCommandSender); + CHIP_ERROR Rollback(CommandSender& aCommandSender); + bool RollbackIsValid() { return mRollbackIsValid; } + private: + TLV::TLVWriter mBackupWriter; + State mBackupState; + bool mRollbackIsValid = false; }; union CallbackHandle diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 5403788385e532..e533959db1f763 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -114,10 +114,10 @@ class ForcedSizeBuffer : public app::DataModel::EncodableToTLV public: ForcedSizeBuffer(uint32_t size) { - if (mBuffer_.Alloc(size)) + if (mBuffer.Alloc(size)) { // No significance with using 0x12, just using a value. - memset(mBuffer_.Get(), 0x12, size); + memset(mBuffer.Get(), 0x12, size); } } @@ -125,17 +125,17 @@ class ForcedSizeBuffer : public app::DataModel::EncodableToTLV static constexpr chip::CommandId GetCommandId() { return 0x12; } CHIP_ERROR EncodeTo(TLV::TLVWriter & aWriter, TLV::Tag aTag) const override { - VerifyOrReturnError(mBuffer_, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(mBuffer, CHIP_ERROR_NO_MEMORY); TLV::TLVType outerContainerType; ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); ReturnErrorOnFailure( - app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer_.Get(), mBuffer_.AllocatedSize()))); + app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize()))); return aWriter.EndContainer(outerContainerType); } private: - chip::Platform::ScopedMemoryBufferWithSize mBuffer_; + chip::Platform::ScopedMemoryBufferWithSize mBuffer; }; struct Fields From 8c9ba744d515864143b574b9213cf11d7d07819f Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 13 Jun 2024 15:24:12 +0000 Subject: [PATCH 06/12] Restyled by clang-format --- src/app/CommandSender.cpp | 10 ++++++---- src/app/CommandSender.h | 5 +++-- src/app/tests/TestCommandInteraction.cpp | 3 +-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 03bb7682964c2d..936bedf8726dcc 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -688,14 +688,16 @@ void CommandSender::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr()); } -void CommandSender::RollbackData::Checkpoint(CommandSender& aCommandSender) { - //InvokeRequestMessage::Builder& aInvokeRequestBuilder, const State& aState) { +void CommandSender::RollbackData::Checkpoint(CommandSender & aCommandSender) +{ + // InvokeRequestMessage::Builder& aInvokeRequestBuilder, const State& aState) { aCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); - mBackupState = aCommandSender.mState; + mBackupState = aCommandSender.mState; mRollbackIsValid = true; } -CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender& aCommandSender) { +CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender & aCommandSender) +{ // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting // InvokeResponses. VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE); diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 92fd054cb13357..213ecf6e532022 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -520,9 +520,10 @@ class CommandSender final : public Messaging::ExchangeDelegate class RollbackData { public: - void Checkpoint(CommandSender& aCommandSender); - CHIP_ERROR Rollback(CommandSender& aCommandSender); + void Checkpoint(CommandSender & aCommandSender); + CHIP_ERROR Rollback(CommandSender & aCommandSender); bool RollbackIsValid() { return mRollbackIsValid; } + private: TLV::TLVWriter mBackupWriter; State mBackupState; diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index e533959db1f763..46f52d106cd3f1 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -129,8 +129,7 @@ class ForcedSizeBuffer : public app::DataModel::EncodableToTLV TLV::TLVType outerContainerType; ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); - ReturnErrorOnFailure( - app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize()))); + ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize()))); return aWriter.EndContainer(outerContainerType); } From ce6a71fd7869cdffbdf9db7218d632de9de8db03 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 13 Jun 2024 18:20:31 +0000 Subject: [PATCH 07/12] Address PR comments --- src/app/CommandSender.cpp | 30 +++++++++--------------------- src/app/CommandSender.h | 35 +++++++++++++++-------------------- 2 files changed, 24 insertions(+), 41 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 936bedf8726dcc..3a44e3d0f1f6d8 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -128,23 +128,6 @@ CHIP_ERROR CommandSender::SendCommandRequestInternal(const SessionHandle & sessi return SendInvokeRequest(); } -void CommandSender::CreateBackupForRequestRollback(RollbackData & aRollbackData) -{ - VerifyOrReturn(mBufferAllocated); - VerifyOrReturn(mState == State::Idle || mState == State::AddedCommand); - VerifyOrReturn(mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); - VerifyOrReturn(mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); - aRollbackData.Checkpoint(*this); -} - -void CommandSender::RollbackRequest(RollbackData & aRollbackData) -{ - VerifyOrReturn(aRollbackData.RollbackIsValid()); - VerifyOrReturn(mState == State::AddingCommand); - ChipLogDetail(DataManagement, "Rolling back response"); - LogErrorOnFailure(aRollbackData.Rollback(*this)); -} - #if CONFIG_BUILD_FOR_HOST_UNIT_TEST CHIP_ERROR CommandSender::TestOnlyCommandSenderTimedRequestFlagWithNoTimedInvoke(const SessionHandle & session, Optional timeout) @@ -560,7 +543,7 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, ReturnErrorOnFailure(AllocateBuffer()); RollbackData rollbackData; - CreateBackupForRequestRollback(rollbackData); + rollbackData.Checkpoint(*this); PrepareCommandParameters prepareCommandParams(aAddRequestDataParams); TLV::TLVWriter * writer = nullptr; CHIP_ERROR err = CHIP_NO_ERROR; @@ -575,7 +558,7 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, exit: if (err != CHIP_NO_ERROR) { - RollbackRequest(rollbackData); + LogErrorOnFailure(rollbackData.Rollback(*this)); } return err; } @@ -690,7 +673,10 @@ void CommandSender::MoveToState(const State aTargetState) void CommandSender::RollbackData::Checkpoint(CommandSender & aCommandSender) { - // InvokeRequestMessage::Builder& aInvokeRequestBuilder, const State& aState) { + VerifyOrReturn(aCommandSender.mBufferAllocated); + VerifyOrReturn(aCommandSender.mState == State::Idle || aCommandSender.mState == State::AddedCommand); + VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); + VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); aCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); mBackupState = aCommandSender.mState; mRollbackIsValid = true; @@ -698,9 +684,11 @@ void CommandSender::RollbackData::Checkpoint(CommandSender & aCommandSender) CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender & aCommandSender) { + VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(aCommandSender.mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + ChipLogDetail(DataManagement, "Rolling back response"); // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting // InvokeResponses. - VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE); aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError(); aCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter); aCommandSender.MoveToState(mBackupState); diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 213ecf6e532022..2dabfcef59e8fa 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -520,7 +520,22 @@ class CommandSender final : public Messaging::ExchangeDelegate class RollbackData { public: + /** + * Creates a backup to enable rolling back CommandSender's buffer containing + * InvokeRequestMessage in case subsequent calls to add request fail. + * + * A successful backup will only be created if the InvokeRequestMessage is + * in a known good state. + * + * @param [in] aCommandSender reference to CommandSender. + */ void Checkpoint(CommandSender & aCommandSender); + /** + * Rolls back CommandSender's buffer containing InvokeRequestMessage to a previously + * saved state. Must have previously called Checkpoint in a known good state. + * + * @param [in] aCommandSender reference to CommandSender. + */ CHIP_ERROR Rollback(CommandSender & aCommandSender); bool RollbackIsValid() { return mRollbackIsValid; } @@ -585,26 +600,6 @@ class CommandSender final : public Messaging::ExchangeDelegate CHIP_ERROR SendCommandRequestInternal(const SessionHandle & session, Optional timeout); - /** - * Creates a backup to enable rolling back buffer containing InvokeRequestMessage - * in case subsequent calls to add request fail. - * - * A successful backup will only be created if the InvokeRequestMessage is - * in a known good state. - * - * @param [in] aRollbackData reference to rollback data that can be on the stack. - */ - void CreateBackupForRequestRollback(RollbackData & aRollbackData); - - /** - * Rolls back buffer containing InvokeRequestMessage to a previously saved state. - * - * @param [in] aRollbackData reference to rollback data that was previously provided - * to CreateBackupForRequestRollback. This rollbackData must be valid for - * a successful rollback. - */ - void RollbackRequest(RollbackData & aRollbackData); - void OnResponseCallback(const ResponseData & aResponseData) { // mpExtendableCallback and mpCallback are mutually exclusive. From f71ec547e590e5a025bf404cb9d9097ff2b3f1e0 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 14 Jun 2024 17:28:00 +0000 Subject: [PATCH 08/12] Address PR comments --- src/app/CommandSender.cpp | 64 +++++++++++++++++++-------------------- src/app/CommandSender.h | 36 +++++++++++----------- 2 files changed, 49 insertions(+), 51 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 3a44e3d0f1f6d8..89cdeed15b0aba 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -542,25 +542,18 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, { ReturnErrorOnFailure(AllocateBuffer()); - RollbackData rollbackData; - rollbackData.Checkpoint(*this); + RollbackInvokeRequest rollback(*this); + //rollback.Checkpoint(*this); PrepareCommandParameters prepareCommandParams(aAddRequestDataParams); + ReturnErrorOnFailure(PrepareCommand(aCommandPath, prepareCommandParams)); TLV::TLVWriter * writer = nullptr; - CHIP_ERROR err = CHIP_NO_ERROR; - SuccessOrExit(err = PrepareCommand(aCommandPath, prepareCommandParams)); writer = GetCommandDataIBTLVWriter(); - VerifyOrExit(writer != nullptr, err = CHIP_ERROR_INCORRECT_STATE); - SuccessOrExit(err = aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields))); - { - FinishCommandParameters finishCommandParams(aAddRequestDataParams); - SuccessOrExit(err = FinishCommand(finishCommandParams)); - } -exit: - if (err != CHIP_NO_ERROR) - { - LogErrorOnFailure(rollbackData.Rollback(*this)); - } - return err; + VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure(aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields))); + FinishCommandParameters finishCommandParams(aAddRequestDataParams); + ReturnErrorOnFailure(FinishCommand(finishCommandParams)); + rollback.DisableAutomaticRollback(); + return CHIP_NO_ERROR; } CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinishCommandParams) @@ -671,29 +664,34 @@ void CommandSender::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr()); } -void CommandSender::RollbackData::Checkpoint(CommandSender & aCommandSender) +CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender& aCommandSender) : + mCommandSender(aCommandSender) { - VerifyOrReturn(aCommandSender.mBufferAllocated); - VerifyOrReturn(aCommandSender.mState == State::Idle || aCommandSender.mState == State::AddedCommand); - VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); - VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); - aCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); - mBackupState = aCommandSender.mState; - mRollbackIsValid = true; + VerifyOrReturn(mCommandSender.mBufferAllocated); + VerifyOrReturn(mCommandSender.mState == State::Idle || mCommandSender.mState == State::AddedCommand); + VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); + VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); + mCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); + mBackupState = mCommandSender.mState; + mRollbackInDestructor = true; } -CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender & aCommandSender) +CommandSender::RollbackInvokeRequest::~RollbackInvokeRequest() { - VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(aCommandSender.mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturn(mRollbackInDestructor); + VerifyOrReturn(mCommandSender.mState == State::AddingCommand); ChipLogDetail(DataManagement, "Rolling back response"); // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting - // InvokeResponses. - aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError(); - aCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter); - aCommandSender.MoveToState(mBackupState); - mRollbackIsValid = false; - return CHIP_NO_ERROR; + // InvokeRequest. + mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError(); + mCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter); + mCommandSender.MoveToState(mBackupState); + mRollbackInDestructor = false; +} + +void CommandSender::RollbackInvokeRequest::DisableAutomaticRollback() +{ + mRollbackInDestructor = false; } } // namespace app diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 2dabfcef59e8fa..f132e72294e7ae 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -517,32 +517,32 @@ class CommandSender final : public Messaging::ExchangeDelegate AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application. }; - class RollbackData + /** + * Class to help backup CommandSender's buffer containing InvokeRequestMessage when adding InvokeRequest + * in case there is a failure to add InvokeRequest. Intended usages is as follows: + * - Allocate RollbackInvokeRequest on the stack. + * - Attempt adding InvokeRequest into InvokeRequestMessage buffer. + * - If modification is added successfully, call DisableAutomaticRollback() to prevent destructor from + * rolling back InvokeReqestMessage. + * - If there is an issue adding InvokeRequest, destructor will take care of rolling back + * InvokeRequestMessage to previously saved state. + */ + class RollbackInvokeRequest { public: + explicit RollbackInvokeRequest(CommandSender& aCommandSender); + ~RollbackInvokeRequest(); + /** - * Creates a backup to enable rolling back CommandSender's buffer containing - * InvokeRequestMessage in case subsequent calls to add request fail. - * - * A successful backup will only be created if the InvokeRequestMessage is - * in a known good state. - * - * @param [in] aCommandSender reference to CommandSender. - */ - void Checkpoint(CommandSender & aCommandSender); - /** - * Rolls back CommandSender's buffer containing InvokeRequestMessage to a previously - * saved state. Must have previously called Checkpoint in a known good state. - * - * @param [in] aCommandSender reference to CommandSender. + * Disables rolling back to previously saved state for InvokeRequestMessage. */ - CHIP_ERROR Rollback(CommandSender & aCommandSender); - bool RollbackIsValid() { return mRollbackIsValid; } + void DisableAutomaticRollback(); private: + CommandSender& mCommandSender; TLV::TLVWriter mBackupWriter; State mBackupState; - bool mRollbackIsValid = false; + bool mRollbackInDestructor = false; }; union CallbackHandle From 1c1bfe03aa49bee287777fb0b52ed5bf8b13bd4e Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 14 Jun 2024 17:32:22 +0000 Subject: [PATCH 09/12] Small fixes --- src/app/CommandSender.cpp | 4 +--- src/app/CommandSender.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 89cdeed15b0aba..30008bc048c509 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -543,11 +543,9 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath, ReturnErrorOnFailure(AllocateBuffer()); RollbackInvokeRequest rollback(*this); - //rollback.Checkpoint(*this); PrepareCommandParameters prepareCommandParams(aAddRequestDataParams); ReturnErrorOnFailure(PrepareCommand(aCommandPath, prepareCommandParams)); - TLV::TLVWriter * writer = nullptr; - writer = GetCommandDataIBTLVWriter(); + TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields))); FinishCommandParameters finishCommandParams(aAddRequestDataParams); diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index f132e72294e7ae..2f36dd18dce698 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -519,7 +519,7 @@ class CommandSender final : public Messaging::ExchangeDelegate /** * Class to help backup CommandSender's buffer containing InvokeRequestMessage when adding InvokeRequest - * in case there is a failure to add InvokeRequest. Intended usages is as follows: + * in case there is a failure to add InvokeRequest. Intended usage is as follows: * - Allocate RollbackInvokeRequest on the stack. * - Attempt adding InvokeRequest into InvokeRequestMessage buffer. * - If modification is added successfully, call DisableAutomaticRollback() to prevent destructor from From 04206bb720a36eab5e0a07994c3869f63401f459 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 14 Jun 2024 17:34:53 +0000 Subject: [PATCH 10/12] Small fixes --- src/app/CommandSender.cpp | 5 ++--- src/app/CommandSender.h | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 30008bc048c509..831b69734614f0 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -662,15 +662,14 @@ void CommandSender::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr()); } -CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender& aCommandSender) : - mCommandSender(aCommandSender) +CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender& aCommandSender) : mCommandSender(aCommandSender) { VerifyOrReturn(mCommandSender.mBufferAllocated); VerifyOrReturn(mCommandSender.mState == State::Idle || mCommandSender.mState == State::AddedCommand); VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR); VerifyOrReturn(mCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR); mCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter); - mBackupState = mCommandSender.mState; + mBackupState = mCommandSender.mState; mRollbackInDestructor = true; } diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 2f36dd18dce698..5ea890a2ebf163 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -530,7 +530,7 @@ class CommandSender final : public Messaging::ExchangeDelegate class RollbackInvokeRequest { public: - explicit RollbackInvokeRequest(CommandSender& aCommandSender); + explicit RollbackInvokeRequest(CommandSender & aCommandSender); ~RollbackInvokeRequest(); /** @@ -539,7 +539,7 @@ class CommandSender final : public Messaging::ExchangeDelegate void DisableAutomaticRollback(); private: - CommandSender& mCommandSender; + CommandSender & mCommandSender; TLV::TLVWriter mBackupWriter; State mBackupState; bool mRollbackInDestructor = false; From bad240b274d9ca6be569be93bf6b8078930b5a32 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 14 Jun 2024 17:35:34 +0000 Subject: [PATCH 11/12] Restyled by clang-format --- src/app/CommandSender.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 831b69734614f0..c19a5d36aca183 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -662,7 +662,7 @@ void CommandSender::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr()); } -CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender& aCommandSender) : mCommandSender(aCommandSender) +CommandSender::RollbackInvokeRequest::RollbackInvokeRequest(CommandSender & aCommandSender) : mCommandSender(aCommandSender) { VerifyOrReturn(mCommandSender.mBufferAllocated); VerifyOrReturn(mCommandSender.mState == State::Idle || mCommandSender.mState == State::AddedCommand); From 0a7af46bf14b5afc3aa95cf35a0a7b7ff294353f Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 14 Jun 2024 19:33:56 +0000 Subject: [PATCH 12/12] Address PR comment --- src/app/CommandSender.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index c19a5d36aca183..dbf1e864cd3ac1 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -679,7 +679,7 @@ CommandSender::RollbackInvokeRequest::~RollbackInvokeRequest() VerifyOrReturn(mCommandSender.mState == State::AddingCommand); ChipLogDetail(DataManagement, "Rolling back response"); // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting - // InvokeRequest. + // InvokeRequests. mCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError(); mCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter); mCommandSender.MoveToState(mBackupState);