Skip to content

Commit

Permalink
Fix session allocation loop when shutting down with an open commissio…
Browse files Browse the repository at this point in the history
…ning window.

After project-chip#20487 if we shut down with a commissioning window open we end up in a
loop where the session manager shutdown marks the tentative PASE session for
eviction, we treat that as a commissioning error and start listening for PASE
again, creating a new session, etc.  With a heap pool this ends up happening to
work in that we keep evicting the new sessions until we hit the 20-attempt limit
and close the commissioning window.  With a non-heap pool, I sort of wonder what
happens, exactly.

The fix here is in two parts, with either part enough on its own to fix the
behavior described above:

1) Shut down the commissioning window manager earlier, before we shut down the
   session manager.  And correspondingly move its initialization during server
   init later.

2) Once session manager starts shutdown, refuse to create any new sessions.
  • Loading branch information
bzbarsky-apple committed Jul 14, 2022
1 parent 37bb2df commit 78c92ec
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
14 changes: 7 additions & 7 deletions src/app/server/Server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,6 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
// TODO(16969): Remove chip::Platform::MemoryInit() call from Server class, it belongs to outer code
chip::Platform::MemoryInit();

SuccessOrExit(err = mCommissioningWindowManager.Init(this));
mCommissioningWindowManager.SetAppDelegate(initParams.appDelegate);

// Initialize PersistentStorageDelegate-based storage
mDeviceStorage = initParams.persistentStorageDelegate;
mSessionResumptionStorage = initParams.sessionResumptionStorage;
Expand Down Expand Up @@ -165,9 +162,6 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
mAclStorage = initParams.aclStorage;
SuccessOrExit(err = mAclStorage->Init(*mDeviceStorage, mFabrics.begin(), mFabrics.end()));

app::DnssdServer::Instance().SetFabricTable(&mFabrics);
app::DnssdServer::Instance().SetCommissioningModeProvider(&mCommissioningWindowManager);

mGroupsProvider = initParams.groupDataProvider;
SetGroupDataProvider(mGroupsProvider);

Expand Down Expand Up @@ -221,6 +215,12 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
err = mUnsolicitedStatusHandler.Init(&mExchangeMgr);
SuccessOrExit(err);

SuccessOrExit(err = mCommissioningWindowManager.Init(this));
mCommissioningWindowManager.SetAppDelegate(initParams.appDelegate);

app::DnssdServer::Instance().SetFabricTable(&mFabrics);
app::DnssdServer::Instance().SetCommissioningModeProvider(&mCommissioningWindowManager);

err = chip::app::InteractionModelEngine::GetInstance()->Init(&mExchangeMgr, &GetFabricTable());
SuccessOrExit(err);

Expand Down Expand Up @@ -424,14 +424,14 @@ void Server::Shutdown()

chip::Dnssd::Resolver::Instance().Shutdown();
chip::app::InteractionModelEngine::GetInstance()->Shutdown();
mCommissioningWindowManager.Shutdown();
mMessageCounterManager.Shutdown();
mExchangeMgr.Shutdown();
mSessions.Shutdown();
mTransports.Close();
mAccessControl.Finish();
Credentials::SetGroupDataProvider(nullptr);
mAttributePersister.Shutdown();
mCommissioningWindowManager.Shutdown();
// TODO(16969): Remove chip::Platform::MemoryInit() call from Server class, it belongs to outer code
chip::Platform::MemoryShutdown();
}
Expand Down
5 changes: 4 additions & 1 deletion src/transport/SessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,16 @@ void SessionManager::Shutdown()
mFabricTable = nullptr;
}

// Ensure that we don't create new sessions as we iterate our session table.
mState = State::kNotReady;

mSecureSessions.ForEachSession([&](auto session) {
session->MarkForEviction();
return Loop::Continue;
});

mMessageCounterManager = nullptr;

mState = State::kNotReady;
mSystemLayer = nullptr;
mTransportMgr = nullptr;
mCB = nullptr;
Expand Down Expand Up @@ -386,6 +388,7 @@ void SessionManager::ExpireAllPASESessions()
Optional<SessionHandle> SessionManager::AllocateSession(SecureSession::Type secureSessionType,
const ScopedNodeId & sessionEvictionHint)
{
VerifyOrReturnValue(mState == State::kInitialized, NullOptional);
return mSecureSessions.CreateNewSecureSession(secureSessionType, sessionEvictionHint);
}

Expand Down

0 comments on commit 78c92ec

Please sign in to comment.