Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release all exchange contexts when shutting down the exchange manager #5692

Merged
merged 4 commits into from
Apr 21, 2021
Merged

Release all exchange contexts when shutting down the exchange manager #5692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59505e2
Release all exchange contexts when shutting down the exchange manager
kghost Mar 30, 2021
128fbbb
Force close ExchangeContexts when shutting down the ExchangeManager
kghost Mar 31, 2021
7883312
Remove useless comment
kghost Apr 8, 2021
a0639a7
Crash when ExchangeContext is leaked
kghost Apr 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/messaging/ExchangeMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ CHIP_ERROR ExchangeManager::Shutdown()
mMessageCounterSyncMgr.Shutdown();
mReliableMessageMgr.Shutdown();

for (auto & ec : mContextPool)
{
// ExchangeContext leaked
assert(ec.GetReferenceCount() == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't really be using assert. I'm not going to block on that, but please file a followup to use something here that does not have the bad codesize effects assert does.

}

if (mSessionMgr != nullptr)
{
mSessionMgr->SetDelegate(nullptr);
Expand Down
2 changes: 0 additions & 2 deletions src/messaging/tests/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ chip_test_suite("tests") {
"MessagingContext.h",
"TestChannel.cpp",
"TestExchangeMgr.cpp",
"TestMessageCounterSyncMgr.cpp",
"TestMessagingLayer.h",
"TestReliableMessageProtocol.cpp",
]
Expand All @@ -49,7 +48,6 @@ chip_test_suite("tests") {
tests = [
"TestChannel",
"TestExchangeMgr",
"TestMessageCounterSyncMgr",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and @kghost what is the plan for re-enabling this test? I figured #5389 would do it, but it doesn't seem to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5389 will enable it.

https://github.com/project-chip/connectedhomeip/pull/5389/files#diff-f3c5e54fca741772049ea3c36beff3eb0f304065a2dbaa381e1ed3ed79de21ad

The file will be rename and moved to src/protocols/message_counter/tests/TestMessageCounterManager.cpp

"TestReliableMessageProtocol",
]
}
8 changes: 7 additions & 1 deletion src/messaging/tests/TestExchangeMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class MockAppDelegate : public ExchangeDelegate
System::PacketBufferHandle buffer) override
{
IsOnMessageReceivedCalled = true;
ec->Close();
}

void OnResponseTimeout(ExchangeContext * ec) override {}
Expand Down Expand Up @@ -102,6 +103,9 @@ void CheckNewContextTest(nlTestSuite * inSuite, void * inContext)
auto sessionLocalToPeer = ctx.GetSecureSessionManager().GetPeerConnectionState(ec2->GetSecureSession());
NL_TEST_ASSERT(inSuite, sessionLocalToPeer->GetPeerNodeId() == ctx.GetDestinationNodeId());
NL_TEST_ASSERT(inSuite, sessionLocalToPeer->GetPeerKeyID() == ctx.GetPeerKeyId());

ec1->Close();
ec2->Close();
}

void CheckUmhRegistrationTest(nlTestSuite * inSuite, void * inContext)
Expand Down Expand Up @@ -157,8 +161,10 @@ void CheckExchangeMessages(nlTestSuite * inSuite, void * inContext)
// send a good packet
ec1->SendMessage(Protocols::Id(VendorId::Common, 0x0001), 0x0001,
System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize),
SendFlags(Messaging::SendMessageFlags::kNone));
SendFlags(Messaging::SendMessageFlags::kNoAutoRequestAck));
NL_TEST_ASSERT(inSuite, mockUnsolicitedAppDelegate.IsOnMessageReceivedCalled);

ec1->Close();
}

// Test Suite
Expand Down
2 changes: 2 additions & 0 deletions src/messaging/tests/TestMessageCounterSyncMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ class MockAppDelegate : public ExchangeDelegate
NL_TEST_ASSERT(mSuite, packetHeader.GetSourceNodeId() == Optional<NodeId>::Value(kSourceNodeId));
NL_TEST_ASSERT(mSuite, packetHeader.GetDestinationNodeId() == Optional<NodeId>::Value(kDestinationNodeId));
NL_TEST_ASSERT(mSuite, msgBuf->DataLength() == kMsgCounterChallengeSize);

ec->Close();
}

void OnResponseTimeout(ExchangeContext * ec) override {}
Expand Down
9 changes: 9 additions & 0 deletions src/messaging/tests/TestReliableMessageProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ void CheckAddClearRetrans(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);
rm->ClearRetransTable(*entry);
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);

exchange->Close();
}

void CheckFailRetrans(nlTestSuite * inSuite, void * inContext)
Expand All @@ -146,6 +148,8 @@ void CheckFailRetrans(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);
rm->FailRetransTableEntries(rc, CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);

exchange->Close();
}

void CheckResendMessage(nlTestSuite * inSuite, void * inContext)
Expand Down Expand Up @@ -192,6 +196,9 @@ void CheckResendMessage(nlTestSuite * inSuite, void * inContext)
test_os_sleep_ms(65);
ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR);
NL_TEST_ASSERT(inSuite, gSendMessageCount == 3);

rm->ClearRetransTable(rc);
exchange->Close();
}

void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext)
Expand All @@ -210,6 +217,8 @@ void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, rc != nullptr);

NL_TEST_ASSERT(inSuite, rc->SendStandaloneAckMessage() == CHIP_NO_ERROR);

exchange->Close();
}

// Test Suite
Expand Down