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

Reduce dangling session handle #9225

Merged
merged 1 commit into from
Sep 2, 2021
Merged

Reduce dangling session handle #9225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 2 additions & 9 deletions src/app/CommandSender.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ using GeneralStatusCode = chip::Protocols::SecureChannel::GeneralStatusCode;
namespace chip {
namespace app {

CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * secureSession,
CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> secureSession,
uint32_t timeout)
{
CHIP_ERROR err = CHIP_NO_ERROR;
Expand All @@ -50,14 +50,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId, FabricIndex aFabric
AbortExistingExchangeContext();

// Create a new exchange context.
if (secureSession == nullptr)
{
mpExchangeCtx = mpExchangeMgr->NewContext(SessionHandle(aNodeId, 0, 0, aFabricIndex), this);
}
else
{
mpExchangeCtx = mpExchangeMgr->NewContext(*secureSession, this);
}
mpExchangeCtx = mpExchangeMgr->NewContext(secureSession.ValueOr(SessionHandle(aNodeId, 0, 0, aFabricIndex)), this);
VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
mpExchangeCtx->SetResponseTimeout(timeout);

Expand Down
2 changes: 1 addition & 1 deletion src/app/CommandSender.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class CommandSender : public Command, public Messaging::ExchangeDelegate
//
// If SendCommandRequest is never called, or the call fails, the API
// consumer is responsible for calling Shutdown on the CommandSender.
CHIP_ERROR SendCommandRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * secureSession,
CHIP_ERROR SendCommandRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> secureSession,
uint32_t timeout = kImMessageTimeoutMsec);

private:
Expand Down
1 change: 0 additions & 1 deletion src/app/ReadHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload)
if (IsInitialReport())
{
VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
mSecureHandle = mpExchangeCtx->GetSecureSession();
}
VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
MoveToState(HandlerState::Reporting);
Expand Down
1 change: 0 additions & 1 deletion src/app/ReadHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ class ReadHandler : public Messaging::ExchangeDelegate
Messaging::ExchangeManager * mpExchangeMgr = nullptr;
InteractionModelDelegate * mpDelegate = nullptr;
bool mInitialReport = false;
SessionHandle mSecureHandle;
};
} // namespace app
} // namespace chip
5 changes: 2 additions & 3 deletions src/app/ReadPrepareParams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,9 @@ struct ReadPrepareParams
uint16_t mMinIntervalSeconds = 0;
uint16_t mMaxIntervalSeconds = 0;

ReadPrepareParams() {}
ReadPrepareParams(ReadPrepareParams && other)
ReadPrepareParams(SessionHandle sessionHandle) : mSessionHandle(sessionHandle) {}
ReadPrepareParams(ReadPrepareParams && other) : mSessionHandle(other.mSessionHandle)
{
mSessionHandle = other.mSessionHandle;
mpEventPathParamsList = other.mpEventPathParamsList;
mEventPathParamsListSize = other.mEventPathParamsListSize;
mpAttributePathParamsList = other.mpAttributePathParamsList;
Expand Down
13 changes: 3 additions & 10 deletions src/app/WriteClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ void WriteClient::ClearState()
MoveToState(State::Uninitialized);
}

CHIP_ERROR WriteClient::SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession,
CHIP_ERROR WriteClient::SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> apSecureSession,
uint32_t timeout)
{
CHIP_ERROR err = CHIP_NO_ERROR;
Expand All @@ -263,14 +263,7 @@ CHIP_ERROR WriteClient::SendWriteRequest(NodeId aNodeId, FabricIndex aFabricInde
ClearExistingExchangeContext();

// Create a new exchange context.
if (apSecureSession == nullptr)
{
mpExchangeCtx = mpExchangeMgr->NewContext(SessionHandle(aNodeId, 0, 0, aFabricIndex), this);
}
else
{
mpExchangeCtx = mpExchangeMgr->NewContext(*apSecureSession, this);
}
mpExchangeCtx = mpExchangeMgr->NewContext(apSecureSession.ValueOr(SessionHandle(aNodeId, 0, 0, aFabricIndex)), this);
VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
mpExchangeCtx->SetResponseTimeout(timeout);

Expand Down Expand Up @@ -397,7 +390,7 @@ CHIP_ERROR WriteClient::ProcessAttributeStatusElement(AttributeStatusElement::Pa
return err;
}

CHIP_ERROR WriteClientHandle::SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession,
CHIP_ERROR WriteClientHandle::SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> apSecureSession,
uint32_t timeout)
{
CHIP_ERROR err = mpWriteClient->SendWriteRequest(aNodeId, aFabricIndex, apSecureSession, timeout);
Expand Down
5 changes: 3 additions & 2 deletions src/app/WriteClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ class WriteClient : public Messaging::ExchangeDelegate
* If SendWriteRequest is never called, or the call fails, the API
* consumer is responsible for calling Shutdown on the WriteClient.
*/
CHIP_ERROR SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession, uint32_t timeout);
CHIP_ERROR SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> apSecureSession,
uint32_t timeout);

/**
* Initialize the client object. Within the lifetime
Expand Down Expand Up @@ -175,7 +176,7 @@ class WriteClientHandle
* Finalize the message and send it to the desired node. The underlying write object will always be released, and the user
* should not use this object after calling this function.
*/
CHIP_ERROR SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession,
CHIP_ERROR SendWriteRequest(NodeId aNodeId, FabricIndex aFabricIndex, Optional<SessionHandle> apSecureSession,
uint32_t timeout = kImMessageTimeoutMsec);

/**
Expand Down
4 changes: 2 additions & 2 deletions src/app/tests/TestCommandInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ void TestCommandInteraction::TestCommandSenderWithWrongState(nlTestSuite * apSui
err = commandSender.Init(&gExchangeManager, nullptr);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

err = commandSender.SendCommandRequest(kTestDeviceNodeId, gFabricIndex, nullptr);
err = commandSender.SendCommandRequest(kTestDeviceNodeId, gFabricIndex, Optional<SessionHandle>::Missing());
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);
}

Expand Down Expand Up @@ -270,7 +270,7 @@ void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSu
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

AddCommandDataElement(apSuite, apContext, &commandSender, false);
err = commandSender.SendCommandRequest(kTestDeviceNodeId, gFabricIndex, nullptr);
err = commandSender.SendCommandRequest(kTestDeviceNodeId, gFabricIndex, Optional<SessionHandle>::Missing());
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NOT_CONNECTED);

GenerateReceivedCommand(apSuite, apContext, buf, true /*aNeedCommandData*/);
Expand Down
29 changes: 13 additions & 16 deletions src/app/tests/TestReadInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,8 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext
System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
err = readClient.Init(&ctx.GetExchangeManager(), &delegate, 0 /* application identifier */);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ReadPrepareParams readPrepareParams;
readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer();
err = readClient.SendReadRequest(readPrepareParams);
ReadPrepareParams readPrepareParams(ctx.GetSessionLocalToPeer());
err = readClient.SendReadRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

GenerateReportData(apSuite, apContext, buf);
Expand All @@ -329,9 +328,8 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &delegate);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Messaging::ExchangeManager exchangeManager;
Messaging::ExchangeContext * exchangeCtx = exchangeManager.NewContext(SessionHandle(), nullptr);
readHandler.Init(&exchangeManager, nullptr, exchangeCtx);
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToPeer(nullptr);
readHandler.Init(&ctx.GetExchangeManager(), nullptr, exchangeCtx);

GenerateReportData(apSuite, apContext, reportDatabuf);
err = readHandler.SendReportData(std::move(reportDatabuf));
Expand Down Expand Up @@ -364,6 +362,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
err = readHandler.OnReadInitialRequest(std::move(readRequestbuf));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

exchangeCtx->Close();
engine->Shutdown();
}

Expand Down Expand Up @@ -431,9 +430,8 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi
err = readClient.Init(&ctx.GetExchangeManager(), &delegate, 0 /* application identifier */);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ReadPrepareParams readPrepareParams;
readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer();
err = readClient.SendReadRequest(readPrepareParams);
ReadPrepareParams readPrepareParams(ctx.GetSessionLocalToPeer());
err = readClient.SendReadRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/);
Expand All @@ -458,9 +456,8 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu
auto * engine = chip::app::InteractionModelEngine::GetInstance();
err = engine->Init(&ctx.GetExchangeManager(), &delegate);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Messaging::ExchangeManager exchangeManager;
Messaging::ExchangeContext * exchangeCtx = exchangeManager.NewContext(SessionHandle(), nullptr);
readHandler.Init(&exchangeManager, nullptr, exchangeCtx);
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToPeer(nullptr);
readHandler.Init(&ctx.GetExchangeManager(), nullptr, exchangeCtx);

GenerateReportData(apSuite, apContext, reportDatabuf);
err = readHandler.SendReportData(std::move(reportDatabuf));
Expand Down Expand Up @@ -488,6 +485,8 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu

err = readHandler.OnReadInitialRequest(std::move(readRequestbuf));
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

exchangeCtx->Close();
engine->Shutdown();
}

Expand Down Expand Up @@ -638,8 +637,7 @@ void TestReadInteraction::TestReadRoundtrip(nlTestSuite * apSuite, void * apCont
attributePathParams[1].mFlags.Set(chip::app::AttributePathParams::Flags::kFieldIdValid);
attributePathParams[1].mFlags.Set(chip::app::AttributePathParams::Flags::kListIndexValid);

ReadPrepareParams readPrepareParams;
readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer();
ReadPrepareParams readPrepareParams(ctx.GetSessionLocalToPeer());
readPrepareParams.mpEventPathParamsList = eventPathParams;
readPrepareParams.mEventPathParamsListSize = 2;
readPrepareParams.mpAttributePathParamsList = attributePathParams;
Expand Down Expand Up @@ -684,8 +682,7 @@ void TestReadInteraction::TestReadInvalidAttributePathRoundtrip(nlTestSuite * ap
attributePathParams[0].mListIndex = 0;
attributePathParams[0].mFlags.Set(chip::app::AttributePathParams::Flags::kFieldIdValid);

ReadPrepareParams readPrepareParams;
readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer();
ReadPrepareParams readPrepareParams(ctx.GetSessionLocalToPeer());
readPrepareParams.mpAttributePathParamsList = attributePathParams;
readPrepareParams.mAttributePathParamsListSize = 1;
err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest(readPrepareParams);
Expand Down
5 changes: 3 additions & 2 deletions src/app/tests/TestWriteInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ void TestWriteInteraction::TestWriteClient(nlTestSuite * apSuite, void * apConte
AddAttributeDataElement(apSuite, apContext, writeClientHandle);

SessionHandle session = ctx.GetSessionLocalToPeer();
err = writeClientHandle.SendWriteRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), &session);
err = writeClientHandle.SendWriteRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(),
Optional<SessionHandle>::Value(session));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
// The internal WriteClient should be nullptr once we SendWriteRequest.
NL_TEST_ASSERT(apSuite, nullptr == writeClientHandle.mpWriteClient);
Expand Down Expand Up @@ -306,7 +307,7 @@ void TestWriteInteraction::TestWriteRoundtrip(nlTestSuite * apSuite, void * apCo

SessionHandle session = ctx.GetSessionLocalToPeer();

err = writeClient.SendWriteRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), &session);
err = writeClient.SendWriteRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), Optional<SessionHandle>::Value(session));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

NL_TEST_ASSERT(apSuite, delegate.mGotResponse);
Expand Down
12 changes: 7 additions & 5 deletions src/app/tests/integration/chip_im_initiator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ CHIP_ERROR SendCommandRequest(chip::app::CommandSender * commandSender)
err = commandSender->FinishCommand();
SuccessOrExit(err);

err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gFabricIndex, nullptr, gMessageTimeoutMsec);
err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gFabricIndex, chip::Optional<chip::SessionHandle>::Missing(),
gMessageTimeoutMsec);
SuccessOrExit(err);

exit:
Expand Down Expand Up @@ -163,7 +164,8 @@ CHIP_ERROR SendBadCommandRequest(chip::app::CommandSender * commandSender)
err = commandSender->FinishCommand();
SuccessOrExit(err);

err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gFabricIndex, nullptr, gMessageTimeoutMsec);
err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gFabricIndex, chip::Optional<chip::SessionHandle>::Missing(),
gMessageTimeoutMsec);
SuccessOrExit(err);

exit:
Expand All @@ -181,7 +183,6 @@ CHIP_ERROR SendBadCommandRequest(chip::app::CommandSender * commandSender)
CHIP_ERROR SendReadRequest()
{
CHIP_ERROR err = CHIP_NO_ERROR;
chip::app::ReadPrepareParams readPrepareParams;
chip::app::EventPathParams eventPathParams[2];
eventPathParams[0].mNodeId = kTestNodeId;
eventPathParams[0].mEndpointId = kTestEndpointId;
Expand All @@ -198,7 +199,7 @@ CHIP_ERROR SendReadRequest()

printf("\nSend read request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId);

readPrepareParams.mSessionHandle = chip::SessionHandle(chip::kTestDeviceNodeId, 0, 0, gFabricIndex);
chip::app::ReadPrepareParams readPrepareParams(chip::SessionHandle(chip::kTestDeviceNodeId, 0, 0, gFabricIndex));
readPrepareParams.mTimeout = gMessageTimeoutMsec;
readPrepareParams.mpAttributePathParamsList = &attributePathParams;
readPrepareParams.mAttributePathParamsListSize = 1;
Expand Down Expand Up @@ -241,7 +242,8 @@ CHIP_ERROR SendWriteRequest(chip::app::WriteClientHandle & apWriteClient)

SuccessOrExit(err = writer->PutBoolean(chip::TLV::ContextTag(chip::app::AttributeDataElement::kCsTag_Data), true));
SuccessOrExit(err = apWriteClient->FinishAttribute());
SuccessOrExit(err = apWriteClient.SendWriteRequest(chip::kTestDeviceNodeId, gFabricIndex, nullptr, gMessageTimeoutMsec));
SuccessOrExit(err = apWriteClient.SendWriteRequest(chip::kTestDeviceNodeId, gFabricIndex,
chip::Optional<chip::SessionHandle>::Missing(), gMessageTimeoutMsec));

gWriteCount++;

Expand Down
3 changes: 2 additions & 1 deletion src/channel/ChannelContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ void ChannelContext::EnterCasePairingState()
auto & prepare = GetPrepareVars();
prepare.mCasePairingSession = Platform::New<CASESession>();

ExchangeContext * ctxt = mExchangeManager->NewContext(SessionHandle(), prepare.mCasePairingSession);
ExchangeContext * ctxt =
mExchangeManager->NewContext(SessionHandle::TemporaryUnauthenticatedSession(), prepare.mCasePairingSession);
VerifyOrReturn(ctxt != nullptr);

// TODO: currently only supports IP/UDP paring
Expand Down
Loading