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

Fix CHIPDeviceControllerFactory shutdown #19022

Merged
merged 1 commit into from
Jun 23, 2022
+58 −39
Merged

Fix CHIPDeviceControllerFactory shutdown #19022

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
44 changes: 23 additions & 21 deletions src/controller/CHIPDeviceControllerFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)

if (mSystemState != nullptr)
{
mSystemState->Release();
chip::Platform::Delete(mSystemState);
Platform::Delete(mSystemState);
mspang marked this conversation as resolved.
Show resolved Hide resolved
mSystemState = nullptr;
}

Expand Down Expand Up @@ -154,14 +153,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
#endif
));

stateParams.sessionMgr = chip::Platform::New<SessionManager>();
SimpleSessionResumptionStorage * sessionResumptionStorage = chip::Platform::New<SimpleSessionResumptionStorage>();
stateParams.sessionResumptionStorage = sessionResumptionStorage;
stateParams.certificateValidityPolicy = params.certificateValidityPolicy;
stateParams.unsolicitedStatusHandler = Platform::New<Protocols::SecureChannel::UnsolicitedStatusHandler>();
stateParams.exchangeMgr = chip::Platform::New<Messaging::ExchangeManager>();
stateParams.messageCounterManager = chip::Platform::New<secure_channel::MessageCounterManager>();
stateParams.groupDataProvider = params.groupDataProvider;
stateParams.sessionMgr = chip::Platform::New<SessionManager>();
stateParams.certificateValidityPolicy = params.certificateValidityPolicy;
stateParams.unsolicitedStatusHandler = Platform::New<Protocols::SecureChannel::UnsolicitedStatusHandler>();
stateParams.exchangeMgr = chip::Platform::New<Messaging::ExchangeManager>();
stateParams.messageCounterManager = chip::Platform::New<secure_channel::MessageCounterManager>();
stateParams.groupDataProvider = params.groupDataProvider;

// This is constructed with a base class deleter so we can std::move it into
// stateParams without a manual conversion below.
auto sessionResumptionStorage =
std::unique_ptr<SimpleSessionResumptionStorage, Platform::Deleter<chip::SessionResumptionStorage>>(
mspang marked this conversation as resolved.
Show resolved Hide resolved
Platform::New<SimpleSessionResumptionStorage>());

// if no fabricTable was provided, create one and track it in stateParams for cleanup
FabricTable * tempFabricTable = nullptr;
Expand All @@ -172,6 +175,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
ReturnErrorOnFailure(stateParams.fabricTable->Init(params.fabricIndependentStorage, params.operationalKeystore));
}
ReturnErrorOnFailure(sessionResumptionStorage->Init(params.fabricIndependentStorage));
stateParams.sessionResumptionStorage = std::move(sessionResumptionStorage);

auto delegate = chip::Platform::MakeUnique<ControllerFabricDelegate>();
ReturnErrorOnFailure(delegate->Init(stateParams.sessionMgr, stateParams.groupDataProvider));
Expand All @@ -198,7 +202,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)

// Enable listening for session establishment messages.
ReturnErrorOnFailure(stateParams.caseServer->ListenForSessionEstablishment(
stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, stateParams.sessionResumptionStorage,
stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, stateParams.sessionResumptionStorage.get(),
stateParams.certificateValidityPolicy, stateParams.groupDataProvider));

//
Expand Down Expand Up @@ -232,7 +236,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)

DeviceProxyInitParams deviceInitParams = {
.sessionManager = stateParams.sessionMgr,
.sessionResumptionStorage = stateParams.sessionResumptionStorage,
.sessionResumptionStorage = stateParams.sessionResumptionStorage.get(),
.exchangeMgr = stateParams.exchangeMgr,
.fabricTable = stateParams.fabricTable,
.clientPool = stateParams.caseClientPool,
Expand All @@ -250,7 +254,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
ReturnErrorOnFailure(stateParams.caseSessionManager->Init(stateParams.systemLayer, sessionManagerConfig));

// store the system state
mSystemState = chip::Platform::New<DeviceControllerSystemState>(stateParams);
mSystemState = chip::Platform::New<DeviceControllerSystemState>(std::move(stateParams));
mSystemState->SetTempFabricTable(tempFabricTable);
ChipLogDetail(Controller, "System State Initialized...");
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -321,21 +325,20 @@ void DeviceControllerFactory::Shutdown()
{
if (mSystemState != nullptr)
{
mSystemState->Release();
chip::Platform::Delete(mSystemState);
Platform::Delete(mSystemState);
mspang marked this conversation as resolved.
Show resolved Hide resolved
mSystemState = nullptr;
}
mFabricIndependentStorage = nullptr;
mOperationalKeystore = nullptr;
}

CHIP_ERROR DeviceControllerSystemState::Shutdown()
void DeviceControllerSystemState::Shutdown()
{
VerifyOrReturnError(mRefCount <= 1, CHIP_ERROR_INCORRECT_STATE);
VerifyOrDie(mRefCount == 0);
if (mHaveShutDown)
{
// Nothing else to do here.
return CHIP_NO_ERROR;
return;
}
mHaveShutDown = true;

Expand Down Expand Up @@ -405,7 +408,8 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
// Consumers are expected to call PlaformMgr().StopEventLoopTask() before calling
// DeviceController::Shutdown() in the CONFIG_DEVICE_LAYER configuration
//
ReturnErrorOnFailure(DeviceLayer::PlatformMgr().Shutdown());
CHIP_ERROR error = DeviceLayer::PlatformMgr().Shutdown();
VerifyOrDie(error == CHIP_NO_ERROR);
#endif

if (mExchangeMgr != nullptr)
Expand Down Expand Up @@ -459,8 +463,6 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
// so that SetupController/Commissioner can use it
mFabrics = nullptr;
}

return CHIP_NO_ERROR;
}

} // namespace Controller
Expand Down
10 changes: 8 additions & 2 deletions src/controller/CHIPDeviceControllerFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,12 @@ class DeviceControllerFactory
}

CHIP_ERROR Init(FactoryInitParams params);

// Shuts down matter and frees the system state.
//
// Must not be called while any controllers are alive.
void Shutdown();

CHIP_ERROR SetupController(SetupParams params, DeviceController & controller);
CHIP_ERROR SetupCommissioner(SetupParams params, DeviceCommissioner & commissioner);

Expand All @@ -135,8 +140,9 @@ class DeviceControllerFactory
//
// Some clients do not prefer a complete shutdown of the stack being initiated if
// all device controllers have ceased to exist. To avoid that, this method has been
// created to permit retention of the underlying system state to avoid that.
// created to permit retention of the underlying system state.
//
// NB: The system state will still be freed in Shutdown() regardless of this call.
void RetainSystemState() { (void) mSystemState->Retain(); }

//
Expand Down Expand Up @@ -212,7 +218,7 @@ class DeviceControllerFactory
};

private:
DeviceControllerFactory(){};
DeviceControllerFactory() {}
void PopulateInitParams(ControllerInitParams & controllerParams, const SetupParams & params);
CHIP_ERROR InitSystemState(FactoryInitParams params);
CHIP_ERROR InitSystemState();
Expand Down
43 changes: 27 additions & 16 deletions src/controller/CHIPDeviceControllerSystemState.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ struct DeviceControllerSystemStateParams

// Params that will be deallocated via Platform::Delete in
// DeviceControllerSystemState::Shutdown.
DeviceTransportMgr * transportMgr = nullptr;
SessionResumptionStorage * sessionResumptionStorage = nullptr;
DeviceTransportMgr * transportMgr = nullptr;
Platform::UniquePtr<SessionResumptionStorage> sessionResumptionStorage;
Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr;
SessionManager * sessionMgr = nullptr;
Protocols::SecureChannel::UnsolicitedStatusHandler * unsolicitedStatusHandler = nullptr;
Expand All @@ -97,9 +97,13 @@ struct DeviceControllerSystemStateParams
FabricTable::Delegate * fabricTableDelegate = nullptr;
};

// A representation of the internal state maintained by the DeviceControllerFactory
// and refcounted by Device Controllers.
// Expects that the creator of this object is the last one to release it.
// A representation of the internal state maintained by the DeviceControllerFactory.
//
// This class automatically maintains a count of active device controllers and
// shuts down Matter when there are none remaining.
//
// NB: Lifetime of the object itself is not managed by reference counting; it is
// owned by DeviceControllerFactory.
class DeviceControllerSystemState
{
using OperationalDevicePool = DeviceControllerSystemStateParams::OperationalDevicePool;
Expand All @@ -113,41 +117,47 @@ class DeviceControllerSystemState
// above 1. In that case we need to make sure we call Shutdown().
Shutdown();
};

DeviceControllerSystemState(DeviceControllerSystemStateParams params) :
mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager),
mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr),
mUnsolicitedStatusHandler(params.unsolicitedStatusHandler), mExchangeMgr(params.exchangeMgr),
mMessageCounterManager(params.messageCounterManager), mFabrics(params.fabricTable), mCASEServer(params.caseServer),
mCASESessionManager(params.caseSessionManager), mOperationalDevicePool(params.operationalDevicePool),
mCASEClientPool(params.caseClientPool), mGroupDataProvider(params.groupDataProvider),
mFabricTableDelegate(params.fabricTableDelegate)
mFabricTableDelegate(params.fabricTableDelegate), mSessionResumptionStorage(std::move(params.sessionResumptionStorage))
{
#if CONFIG_NETWORK_LAYER_BLE
mBleLayer = params.bleLayer;
#endif
VerifyOrDie(IsInitialized());
};

// Acquires a reference to the system state.
//
// While a reference is held, the shared state is kept alive. Release()
// should be called to release the reference once it is no longer needed.
DeviceControllerSystemState * Retain()
{
VerifyOrDie(mRefCount < std::numeric_limits<uint32_t>::max());
++mRefCount;
return this;
};

// Releases a reference to the system state.
//
// The stack will shut down when all references are released.
//
// NB: The system state is owned by the factory; Relase() will not free it
// but will free its members (Shutdown()).
void Release()
{
VerifyOrDie(mRefCount > 0);

mRefCount--;
if (mRefCount == 1)
if (--mRefCount == 0)
{
// Only the factory should have a ref now, shutdown and release the underlying objects
Shutdown();
mspang marked this conversation as resolved.
Show resolved Hide resolved
}
else if (mRefCount == 0)
{
this->~DeviceControllerSystemState();
}
};
bool IsInitialized()
{
Expand All @@ -173,7 +183,7 @@ class DeviceControllerSystemState
void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; }

private:
DeviceControllerSystemState(){};
DeviceControllerSystemState() {}

System::Layer * mSystemLayer = nullptr;
Inet::EndPointManager<Inet::TCPEndPoint> * mTCPEndPointManager = nullptr;
Expand All @@ -193,17 +203,18 @@ class DeviceControllerSystemState
CASEClientPool * mCASEClientPool = nullptr;
Credentials::GroupDataProvider * mGroupDataProvider = nullptr;
FabricTable::Delegate * mFabricTableDelegate = nullptr;
Platform::UniquePtr<SessionResumptionStorage> mSessionResumptionStorage;

// If mTempFabricTable is not null, it was created during
// DeviceControllerFactory::InitSystemState and needs to be
// freed during shutdown
FabricTable * mTempFabricTable = nullptr;

std::atomic<uint32_t> mRefCount{ 1 };
std::atomic<uint32_t> mRefCount{ 0 };

bool mHaveShutDown = false;

CHIP_ERROR Shutdown();
void Shutdown();
};

} // namespace Controller
Expand Down