Skip to content

Commit

Permalink
Fix status reponse in write client
Browse files Browse the repository at this point in the history
  • Loading branch information
@yunhanw-google
yunhanw-google committed Aug 2, 2022
1 parent c902d88 commit 958f244
Show file tree
Hide file tree
Showing 2 changed files with 304 additions and 9 deletions.
30 changes: 22 additions & 8 deletions src/app/WriteClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,22 +423,30 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
MoveToState(State::ResponseReceived);
}

CHIP_ERROR err = CHIP_NO_ERROR;

CHIP_ERROR err = CHIP_NO_ERROR;
bool sendStatusResponse = false;
// Assert that the exchange context matches the client's current context.
// This should never fail because even if SendWriteRequest is called
// back-to-back, the second call will call Close() on the first exchange,
// which clears the OnMessageReceived callback.
VerifyOrExit(apExchangeContext == mExchangeCtx.Get(), err = CHIP_ERROR_INCORRECT_STATE);

sendStatusResponse = true;

if (mState == State::AwaitingTimedStatus)
{
VerifyOrExit(aPayloadHeader.HasMessageType(MsgType::StatusResponse), err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
CHIP_ERROR statusError = CHIP_NO_ERROR;
SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
SuccessOrExit(err = statusError);
err = SendWriteRequest();

if (aPayloadHeader.HasMessageType(MsgType::StatusResponse))
{
CHIP_ERROR statusError = CHIP_NO_ERROR;
SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
sendStatusResponse = false;
SuccessOrExit(err = statusError);
err = SendWriteRequest();
}
else
{
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}
// Skip all other processing here (which is for the response to the
// write request), no matter whether err is success or not.
goto exit;
Expand All @@ -448,6 +456,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
{
err = ProcessWriteResponseMessage(std::move(aPayload));
SuccessOrExit(err);
sendStatusResponse = false;
if (!mChunks.IsNull())
{
// Send the next chunk.
Expand Down Expand Up @@ -475,6 +484,11 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
}
}

if (sendStatusResponse)
{
StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
}

if (mState != State::AwaitingResponse)
{
Close();
Expand Down
283 changes: 282 additions & 1 deletion src/app/tests/TestWriteInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ class TestWriteInteraction
static void TestWriteClientGroup(nlTestSuite * apSuite, void * apContext);
static void TestWriteHandler(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtrip(nlTestSuite * apSuite, void * apContext);
static void TestWriteInvalidMessage1(nlTestSuite * apSuite, void * apContext);
static void TestWriteInvalidMessage2(nlTestSuite * apSuite, void * apContext);
static void TestWriteInvalidMessage3(nlTestSuite * apSuite, void * apContext);
static void TestWriteInvalidMessage4(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtripWithClusterObjects(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtripWithClusterObjectsVersionMatch(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext);
Expand Down Expand Up @@ -90,13 +94,18 @@ class TestWriteClientCallback : public chip::app::WriteClient::Callback
mStatus = status;
mOnSuccessCalled++;
}
void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override { mOnErrorCalled++; }
void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override
{
mError = chipError;
mOnErrorCalled++;
}
void OnDone(WriteClient * apWriteClient) override { mOnDoneCalled++; }

int mOnSuccessCalled = 0;
int mOnErrorCalled = 0;
int mOnDoneCalled = 0;
StatusIB mStatus;
CHIP_ERROR mError = CHIP_NO_ERROR;
};

void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient)
Expand Down Expand Up @@ -543,6 +552,274 @@ void TestWriteInteraction::TestWriteRoundtrip(nlTestSuite * apSuite, void * apCo
engine->Shutdown();
}

// Write Client sends the write request, and process the unknown message error via OnMessageReceived to close the client.
void TestWriteInteraction::TestWriteInvalidMessage1(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);

CHIP_ERROR err = CHIP_NO_ERROR;

Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
// Shouldn't have anything in the retransmit table when starting the test.
NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);

TestWriteClientCallback callback;
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional<uint16_t>::Missing());

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
AddAttributeDataIB(apSuite, apContext, writeClient);

NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 0 && callback.mOnDoneCalled == 0);

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 3;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 1;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.SendWriteRequest(ctx.GetSessionBobToAlice());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1);

System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
System::PacketBufferTLVWriter writer;
writer.Init(std::move(msgBuf));
ReportDataMessage::Builder response;
response.Init(&writer);
NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR);
PayloadHeader payloadHeader;
payloadHeader.SetExchangeID(0);
payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData);

rm->ClearRetransTable(writeClient.mExchangeCtx.Get());
ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 0;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.OnMessageReceived(writeClient.mExchangeCtx.Get(), payloadHeader, std::move(msgBuf));
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_MESSAGE_TYPE);
ctx.DrainAndServiceIO();
NL_TEST_ASSERT(apSuite, callback.mError == CHIP_ERROR_INVALID_MESSAGE_TYPE);
NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 1 && callback.mOnDoneCalled == 1);

// Client sents status report with invalid action, server's exchange has been closed, and it would send MRP Ack
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);

engine->Shutdown();
ctx.ExpireSessionAliceToBob();
ctx.ExpireSessionBobToAlice();
ctx.CreateSessionAliceToBob();
ctx.CreateSessionBobToAlice();
}

// Write Client sends the write request, and process the invalid write response message error via OnMessageReceived to close the
// client.
void TestWriteInteraction::TestWriteInvalidMessage2(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);

CHIP_ERROR err = CHIP_NO_ERROR;

Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
// Shouldn't have anything in the retransmit table when starting the test.
NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);

TestWriteClientCallback callback;
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional<uint16_t>::Missing());

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
AddAttributeDataIB(apSuite, apContext, writeClient);

NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 0 && callback.mOnDoneCalled == 0);

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 3;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 1;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.SendWriteRequest(ctx.GetSessionBobToAlice());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1);

System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
System::PacketBufferTLVWriter writer;
writer.Init(std::move(msgBuf));
WriteResponseMessage::Builder response;
response.Init(&writer);
NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR);
PayloadHeader payloadHeader;
payloadHeader.SetExchangeID(0);
payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::WriteResponse);

rm->ClearRetransTable(writeClient.mExchangeCtx.Get());
ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 0;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.OnMessageReceived(writeClient.mExchangeCtx.Get(), payloadHeader, std::move(msgBuf));
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_END_OF_TLV);
ctx.DrainAndServiceIO();
NL_TEST_ASSERT(apSuite, callback.mError == CHIP_ERROR_END_OF_TLV);
NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 1 && callback.mOnDoneCalled == 1);

// Client sents status report with invalid action, server's exchange has been closed, and it would send MRP Ack
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);

engine->Shutdown();
ctx.ExpireSessionAliceToBob();
ctx.ExpireSessionBobToAlice();
ctx.CreateSessionAliceToBob();
ctx.CreateSessionBobToAlice();
}

// Write Client sends the write request, and process the malformed status response message error via OnMessageReceived to close the
// client.
void TestWriteInteraction::TestWriteInvalidMessage3(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);

CHIP_ERROR err = CHIP_NO_ERROR;

Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
// Shouldn't have anything in the retransmit table when starting the test.
NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);

TestWriteClientCallback callback;
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional<uint16_t>::Missing());

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
AddAttributeDataIB(apSuite, apContext, writeClient);

NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 0 && callback.mOnDoneCalled == 0);

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 3;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 1;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.SendWriteRequest(ctx.GetSessionBobToAlice());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1);

System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
System::PacketBufferTLVWriter writer;
writer.Init(std::move(msgBuf));
StatusResponseMessage::Builder response;
response.Init(&writer);
NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR);
PayloadHeader payloadHeader;
payloadHeader.SetExchangeID(0);
payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse);

rm->ClearRetransTable(writeClient.mExchangeCtx.Get());
ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 0;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.OnMessageReceived(writeClient.mExchangeCtx.Get(), payloadHeader, std::move(msgBuf));
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_END_OF_TLV);
ctx.DrainAndServiceIO();
NL_TEST_ASSERT(apSuite, callback.mError == CHIP_ERROR_END_OF_TLV);
NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 1 && callback.mOnDoneCalled == 1);

// Client sents status report with invalid action, server's exchange has been closed, and it would send MRP Ack
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);

engine->Shutdown();
ctx.ExpireSessionAliceToBob();
ctx.ExpireSessionBobToAlice();
ctx.CreateSessionAliceToBob();
ctx.CreateSessionBobToAlice();
}

// Write Client sends the write request, and process the busy status response message error via OnMessageReceived to close the
// client.
void TestWriteInteraction::TestWriteInvalidMessage4(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);

CHIP_ERROR err = CHIP_NO_ERROR;

Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
// Shouldn't have anything in the retransmit table when starting the test.
NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);

TestWriteClientCallback callback;
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

app::WriteClient writeClient(engine->GetExchangeManager(), &callback, Optional<uint16_t>::Missing());

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
AddAttributeDataIB(apSuite, apContext, writeClient);

NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 0 && callback.mOnDoneCalled == 0);

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 3;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 1;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.SendWriteRequest(ctx.GetSessionBobToAlice());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1);

System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
System::PacketBufferTLVWriter writer;
writer.Init(std::move(msgBuf));
StatusResponseMessage::Builder response;
response.Init(&writer);
response.Status(Protocols::InteractionModel::Status::Busy);
NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR);
PayloadHeader payloadHeader;
payloadHeader.SetExchangeID(0);
payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse);

rm->ClearRetransTable(writeClient.mExchangeCtx.Get());
ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 0;
ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0;
ctx.GetLoopback().mDroppedMessageCount = 0;
err = writeClient.OnMessageReceived(writeClient.mExchangeCtx.Get(), payloadHeader, std::move(msgBuf));
NL_TEST_ASSERT(apSuite, err == CHIP_IM_GLOBAL_STATUS(Busy));
ctx.DrainAndServiceIO();
NL_TEST_ASSERT(apSuite, callback.mError == CHIP_IM_GLOBAL_STATUS(Busy));
NL_TEST_ASSERT(apSuite, callback.mOnSuccessCalled == 0 && callback.mOnErrorCalled == 1 && callback.mOnDoneCalled == 1);

// Client sents status report with invalid action, server's exchange has been closed, and it would send MRP Ack
NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2);

engine->Shutdown();
ctx.ExpireSessionAliceToBob();
ctx.ExpireSessionBobToAlice();
ctx.CreateSessionAliceToBob();
ctx.CreateSessionBobToAlice();
}

} // namespace app
} // namespace chip

Expand All @@ -562,6 +839,10 @@ const nlTest sTests[] =
NL_TEST_DEF("TestWriteRoundtripWithClusterObjects", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjects),
NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMatch),
NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch),
NL_TEST_DEF("TestWriteInvalidMessage1", chip::app::TestWriteInteraction::TestWriteInvalidMessage1),
NL_TEST_DEF("TestWriteInvalidMessage2", chip::app::TestWriteInteraction::TestWriteInvalidMessage2),
NL_TEST_DEF("TestWriteInvalidMessage3", chip::app::TestWriteInteraction::TestWriteInvalidMessage3),
NL_TEST_DEF("TestWriteInvalidMessage4", chip::app::TestWriteInteraction::TestWriteInvalidMessage4),
NL_TEST_SENTINEL()
};
// clang-format on
Expand Down

0 comments on commit 958f244

Please sign in to comment.