Skip to content

Commit

Permalink
Fix TestReliableMessageProtocol timing bug.
Browse files Browse the repository at this point in the history 
We could end up retransmitting a message due to losing the timeslice when we
only meant to send it once.

The fix is to make our stopping and starting of the reliable message manager
timer in the test a lot less ad-hoc: we stop it whenever we block sending an ack
(so that the other side will not retransmit), then restart it when we want to
allow acks again (because at that point we want to make sure we retransmit
things as needed).

Fixes #27810
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Jul 7, 2023
1 parent a2a3712 commit 2b948fb
Showing 1 changed file with 56 additions and 40 deletions.
96 changes: 56 additions & 40 deletions src/messaging/tests/TestReliableMessageProtocol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ constexpr auto retryBoosterTimeout = CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS * CHIP_
class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegate
{
public:
MockAppDelegate(TestContext & ctx) : mTestContext(ctx) {}

CHIP_ERROR OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, ExchangeDelegate *& newDelegate) override
{
// Handle messages by myself
Expand All @@ -89,6 +91,9 @@ class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegat
auto * rc = ec->GetReliableMessageContext();
if (rc->HasPiggybackAckPending())
{
// Make sure we don't accidentally retransmit and end up acking
// the retransmit.
rc->GetReliableMessageMgr()->StopTimer();
(void) rc->TakePendingPeerAckMessageCounter();
}
}
Expand Down Expand Up @@ -127,12 +132,27 @@ class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegat
}
}

void SetDropAckResponse(bool dropResponse)
{
mDropAckResponse = dropResponse;
if (!mDropAckResponse)
{
// Restart the MRP retransmit timer, now that we are not going to be
// dropping acks anymore, so we send out pending retransmits, if
// any, as needed.
mTestContext.GetExchangeManager().GetReliableMessageMgr()->StartTimer();
}
}

bool IsOnMessageReceivedCalled = false;
bool mReceivedPiggybackAck = false;
bool mDropAckResponse = false;
bool mRetainExchange = false;
ExchangeContext * mExchange = nullptr;
nlTestSuite * mTestSuite = nullptr;

private:
TestContext & mTestContext;
bool mDropAckResponse = false;
};

class MockSessionEstablishmentExchangeDispatch : public Messaging::ApplicationExchangeDispatch
Expand Down Expand Up @@ -279,7 +299,7 @@ void CheckAddClearRetrans(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);

MockAppDelegate mockAppDelegate;
MockAppDelegate mockAppDelegate(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockAppDelegate);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -337,7 +357,7 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
// TODO: temporarily create a SessionHandle from node id, will be fix in PR 3602
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);
Expand Down Expand Up @@ -450,7 +470,7 @@ void CheckCloseExchangeAndResendApplicationMessage(nlTestSuite * inSuite, void *

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
// TODO: temporarily create a SessionHandle from node id, will be fixed in PR 3602
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);
Expand Down Expand Up @@ -594,13 +614,13 @@ void CheckResendApplicationMessageWithPeerExchange(nlTestSuite * inSuite, void *

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -656,13 +676,13 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand All @@ -681,8 +701,8 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext
loopback.mDroppedMessageCount = 0;

// Drop the ack, and also close the peer exchange
mockReceiver.mDropAckResponse = true;
mockReceiver.mRetainExchange = false;
mockReceiver.SetDropAckResponse(true);
mockReceiver.mRetainExchange = false;

// Ensure the retransmit table is empty right now
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
Expand All @@ -698,7 +718,7 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);

// Let's not drop the duplicate message
mockReceiver.mDropAckResponse = false;
mockReceiver.SetDropAckResponse(false);

err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
Expand All @@ -723,13 +743,13 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand All @@ -748,8 +768,8 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont
loopback.mDroppedMessageCount = 0;

// Drop the ack, and also close the peer exchange
mockReceiver.mDropAckResponse = true;
mockReceiver.mRetainExchange = false;
mockReceiver.SetDropAckResponse(true);
mockReceiver.mRetainExchange = false;

// Ensure the retransmit table is empty right now
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
Expand All @@ -764,10 +784,6 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont
NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 0);
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);

// Make sure we don't accidentally retransmit before we are done with our
// message counter incrementing.
rm->StopTimer();

// Now send CHIP_CONFIG_MESSAGE_COUNTER_WINDOW_SIZE + 2 messages to make
// sure our original message is out of the message counter window. These
// messages can be sent withour MRP, because we are not expecting acks for
Expand Down Expand Up @@ -798,7 +814,7 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont
}

// Let's not drop the duplicate message's ack.
mockReceiver.mDropAckResponse = false;
mockReceiver.SetDropAckResponse(false);

err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
Expand Down Expand Up @@ -892,13 +908,13 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand All @@ -917,8 +933,8 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext)
loopback.mDroppedMessageCount = 0;

// Drop the ack, and keep the exchange around to receive the duplicate message
mockReceiver.mDropAckResponse = true;
mockReceiver.mRetainExchange = true;
mockReceiver.SetDropAckResponse(true);
mockReceiver.mRetainExchange = true;

// Ensure the retransmit table is empty right now
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
Expand All @@ -937,8 +953,8 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// Let's not drop the duplicate message
mockReceiver.mDropAckResponse = false;
mockReceiver.mRetainExchange = false;
mockReceiver.SetDropAckResponse(false);
mockReceiver.mRetainExchange = false;

// Wait for the first re-transmit and ack (should take 64ms)
ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 3; });
Expand All @@ -962,13 +978,13 @@ void CheckReceiveAfterStandaloneAck(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1052,13 +1068,13 @@ void CheckPiggybackAfterPiggyback(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1191,7 +1207,7 @@ void CheckSendUnsolicitedStandaloneAckMessage(nlTestSuite * inSuite, void * inCo

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1229,7 +1245,7 @@ void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);

MockAppDelegate mockAppDelegate;
MockAppDelegate mockAppDelegate(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockAppDelegate);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1268,13 +1284,13 @@ void CheckMessageAfterClosed(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1388,13 +1404,13 @@ void CheckLostResponseWithPiggyback(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand Down Expand Up @@ -1543,13 +1559,13 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)

CHIP_ERROR err = CHIP_NO_ERROR;

MockAppDelegate mockReceiver;
MockAppDelegate mockReceiver(ctx);
err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

mockReceiver.mTestSuite = inSuite;

MockAppDelegate mockSender;
MockAppDelegate mockSender(ctx);
ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender);
NL_TEST_ASSERT(inSuite, exchange != nullptr);

Expand All @@ -1574,7 +1590,7 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)
mockSender.mRetainExchange = true;

// And ensure the ack heading back our way is dropped.
mockReceiver.mDropAckResponse = true;
mockReceiver.SetDropAckResponse(true);

err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer), SendFlags(SendMessageFlags::kExpectResponse));
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
Expand All @@ -1596,7 +1612,7 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, !receiverRc->IsAckPending());

// Don't drop any more acks.
mockReceiver.mDropAckResponse = false;
mockReceiver.SetDropAckResponse(false);

// Now send a message from the other side.
buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD));
Expand Down

0 comments on commit 2b948fb

Please sign in to comment.