Skip to content

Commit

Permalink
Check and fix System::Layer shutdown sequencing (#9247)
Browse files Browse the repository at this point in the history
#### Problem

There have been multiple instances of code using a `System::Layer`
object after it has been shut down or destroyed, some previously
fixed in #8865. (In the ancestral code, `System::Layer` was fully
static.)

#### Change overview

- More thoroughly track and check `System::Layer` object state.
- Spin out state tracking into `lib/support/ObjectLifeCycle.h`
- Fix several sequencing issues.

#### Testing

CI; no change to visible functionality intended.
  • Loading branch information
kpschoedel authored Aug 31, 2021
1 parent 8be2f3d commit 371fb31
Show file tree
Hide file tree
Showing 16 changed files with 215 additions and 78 deletions.
6 changes: 4 additions & 2 deletions examples/platform/nrfconnect/util/test/TestInetCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,10 @@ void ServiceEvents(struct ::timeval & aSleepTime)
FD_ZERO(&exceptFDs);

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
if (gSystemLayer.State() == System::LayerState::kInitialized)
if (gSystemLayer.IsInitialized())
{
gSystemLayer.PrepareSelect(numFDs, &readFDs, &writeFDs, &exceptFDs, aSleepTime);
}
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
Expand All @@ -130,7 +132,7 @@ void ServiceEvents(struct ::timeval & aSleepTime)
}
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS

if (gSystemLayer.State() == System::LayerState::kInitialized)
if (gSystemLayer.IsInitialized())
{

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
Expand Down
2 changes: 1 addition & 1 deletion src/app/tests/integration/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ void InitializeChip(void)

void ShutdownChip(void)
{
chip::DeviceLayer::PlatformMgr().Shutdown();
gMessageCounterManager.Shutdown();
gExchangeManager.Shutdown();
gSessionManager.Shutdown();
chip::DeviceLayer::PlatformMgr().Shutdown();
}

void TLVPrettyPrinter(const char * aFormat, ...)
Expand Down
40 changes: 20 additions & 20 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,26 @@ CHIP_ERROR DeviceController::Shutdown()
mActiveDevices[i].Reset();
}

mState = State::NotInitialized;

// Shut down the interaction model before we try shuttting down the exchange
// manager.
app::InteractionModelEngine::GetInstance()->Shutdown();

// TODO(#6668): Some exchange has leak, shutting down ExchangeManager will cause a assert fail.
// if (mExchangeMgr != nullptr)
// {
// mExchangeMgr->Shutdown();
// }
if (mSessionMgr != nullptr)
{
mSessionMgr->Shutdown();
}

mStorageDelegate = nullptr;

ReleaseAllDevices();

#if CONFIG_DEVICE_LAYER
//
// We can safely call PlatformMgr().Shutdown(), which like DeviceController::Shutdown(),
Expand All @@ -279,26 +299,6 @@ CHIP_ERROR DeviceController::Shutdown()
mSystemLayer = nullptr;
mInetLayer = nullptr;

mState = State::NotInitialized;

// Shut down the interaction model before we try shuttting down the exchange
// manager.
app::InteractionModelEngine::GetInstance()->Shutdown();

// TODO(#6668): Some exchange has leak, shutting down ExchangeManager will cause a assert fail.
// if (mExchangeMgr != nullptr)
// {
// mExchangeMgr->Shutdown();
// }
if (mSessionMgr != nullptr)
{
mSessionMgr->Shutdown();
}

mStorageDelegate = nullptr;

ReleaseAllDevices();

if (mMessageCounterManager != nullptr)
{
chip::Platform::Delete(mMessageCounterManager);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ CHIP_ERROR GenericPlatformManagerImpl_Zephyr<ImplClass>::_StopEventLoopTask(void
template <class ImplClass>
CHIP_ERROR GenericPlatformManagerImpl_Zephyr<ImplClass>::_Shutdown(void)
{
VerifyOrDieWithMsg(false, DeviceLayer, "Shutdown is not implemented");
return CHIP_ERROR_NOT_IMPLEMENTED;
}

Expand Down
12 changes: 9 additions & 3 deletions src/inet/TCPEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1569,13 +1569,17 @@ CHIP_ERROR TCPEndPoint::DoClose(CHIP_ERROR err, bool suppressCallback)
else
State = kState_Closed;

// Stop the Connect timer in case it is still running.

StopConnectTimer();
if (oldState != kState_Closed)
{
// Stop the Connect timer in case it is still running.
StopConnectTimer();
}

// If not making a state transition, return immediately.
if (State == oldState)
{
return CHIP_NO_ERROR;
}

#if CHIP_SYSTEM_CONFIG_USE_LWIP

Expand Down Expand Up @@ -2755,7 +2759,9 @@ void TCPEndPoint::HandleIncomingConnection()
if (conEP != nullptr)
{
if (conEP->State == kState_Connected)
{
conEP->Release();
}
conEP->Release();
}
if (OnAcceptError != nullptr)
Expand Down
19 changes: 8 additions & 11 deletions src/inet/tests/TestInetCommonPosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -471,22 +471,19 @@ void ServiceEvents(uint32_t aSleepTimeMilliseconds)
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS

#if CHIP_SYSTEM_CONFIG_USE_LWIP
if (gSystemLayer.State() == System::LayerState::kInitialized)
if (gSystemLayer.IsInitialized())
{
static uint32_t sRemainingSystemLayerEventDelay = 0;

if (gSystemLayer.State() == System::LayerState::kInitialized)
if (sRemainingSystemLayerEventDelay == 0)
{
if (sRemainingSystemLayerEventDelay == 0)
{
gSystemLayer.WatchableEventsManager().DispatchEvents();
sRemainingSystemLayerEventDelay = gNetworkOptions.EventDelay;
}
else
sRemainingSystemLayerEventDelay--;

gSystemLayer.WatchableEventsManager().HandlePlatformTimer();
gSystemLayer.WatchableEventsManager().DispatchEvents();
sRemainingSystemLayerEventDelay = gNetworkOptions.EventDelay;
}
else
sRemainingSystemLayerEventDelay--;

gSystemLayer.WatchableEventsManager().HandlePlatformTimer();
}
#if CHIP_TARGET_STYLE_UNIX
// TapAddrAutoconf and TapInterface are only needed for LwIP on
Expand Down
7 changes: 6 additions & 1 deletion src/inet/tests/TestInetEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ static void TestInetPre(nlTestSuite * inSuite, void * inContext)
#endif // INET_CONFIG_ENABLE_DNS_RESOLVER

// Deinit system layer and network
ShutdownSystemLayer();
ShutdownNetwork();
if (gSystemLayer.IsInitialized())
{
ShutdownSystemLayer();
}

#if INET_CONFIG_ENABLE_RAW_ENDPOINT
err = gInet.NewRawEndPoint(kIPVersion_6, kIPProtocol_ICMPv6, &testRawEP);
Expand Down Expand Up @@ -589,6 +592,8 @@ static int TestSetup(void * inContext)
*/
static int TestTeardown(void * inContext)
{
ShutdownNetwork();
ShutdownSystemLayer();
chip::Platform::MemoryShutdown();
return SUCCESS;
}
Expand Down
1 change: 1 addition & 0 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ static_library("support") {
"FixedBufferAllocator.h",
"LifetimePersistedCounter.cpp",
"LifetimePersistedCounter.h",
"ObjectLifeCycle.h",
"PersistedCounter.cpp",
"PersistedCounter.h",
"PersistentStorageMacros.h",
Expand Down
135 changes: 135 additions & 0 deletions src/lib/support/ObjectLifeCycle.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
*
* Copyright (c) 2021 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include <cstdint>

namespace chip {

/**
* Track the life cycle of an object.
*
* <pre>
* ┌───────────────┐ Init ┌─────────────┐ Shutdown ┌──────────┐ Destroy ┌───────────┐
* Construction ───>│ Uninitialized ├───────>│ Initialized ├───────────>│ Shutdown ├──────────>│ Destroyed │
* └──────────┬────┘ └─────────────┘ └─────┬────┘ └───────────┘
* ^ │ Reset │
* └──────┴<─────────────────────────────────────────────┘
* </pre>
*/
struct ObjectLifeCycle
{
/**
* @returns true if and only if the object is in the Initialized state.
*/
bool IsInitialized() const { return mState == State::Initialized; }

/**
* Transition from Uninitialized to Initialized.
*
* Typical use is `VerifyOrReturnError(state.Init(), CHIP_ERROR_INCORRECT_STATE)`; this function returns `bool` rather than
* a `CHIP_ERROR` so that error source tracking will record the call point rather than this function itself.
*
* @return true if the state was Uninitialized and is now Initialized.
* @return false otherwise.
*/
bool Init()
{
if (mState == State::Uninitialized)
{
mState = State::Initialized;
return true;
}
return false;
}

/**
* Transition from Initialized to Shutdown.
*
* Typical use is `VerifyOrReturnError(state.Shutdown(), CHIP_ERROR_INCORRECT_STATE)`; this function returns `bool` rather than
* a `CHIP_ERROR` so that error source tracking will record the call point rather than this function itself.
*
* @return true if the state was Initialized and is now Shutdown.
* @return false otherwise.
*/
bool Shutdown()
{
if (mState == State::Initialized)
{
mState = State::Shutdown;
return true;
}
return false;
}

/**
* Transition from Shutdown back to Uninitialized, or remain Uninitialized.
*
* Typical use is `VerifyOrReturnError(state.Reset(), CHIP_ERROR_INCORRECT_STATE)`; this function returns `bool` rather than
* a `CHIP_ERROR` so that error source tracking will record the call point rather than this function itself.
*
* @return true if the state was Uninitialized or Shutdown and is now Uninitialized.
* @return false otherwise.
*/
bool Reset()
{
if (mState == State::Shutdown)
{
mState = State::Uninitialized;
}
return mState == State::Uninitialized;
}

/**
* Transition from Uninitialized or Shutdown to Destroyed.
*
* Typical use is `VerifyOrReturnError(state.Destroy(), CHIP_ERROR_INCORRECT_STATE)`; this function returns `bool` rather than
* a `CHIP_ERROR` so that error source tracking will record the call point rather than this function itself.
*
* @return true if the state was Uninitialized or Shutdown and is now Destroyed.
* @return false otherwise.
*/
bool Destroy()
{
if (mState == State::Uninitialized || mState == State::Shutdown)
{
mState = State::Destroyed;
return true;
}
return false;
}

/**
* Return the current state as an integer. This is intended for troubleshooting or logging, since there is no code access to
* the meaning of the integer value.
*/
explicit operator int() const { return static_cast<int>(mState); }

private:
enum class State : uint8_t
{
Uninitialized = 0, /**< Pre-initialized state; */
Initialized = 1, /**< Initialized state. */
Shutdown = 2, /**< Post-Shutdown state. */
Destroyed = 3, /**< Post-destructor state. */
};
State mState = State::Uninitialized;
};

} // namespace chip
2 changes: 1 addition & 1 deletion src/messaging/tests/echo/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ void InitializeChip(void)

void ShutdownChip(void)
{
chip::DeviceLayer::PlatformMgr().Shutdown();
gMessageCounterManager.Shutdown();
gExchangeManager.Shutdown();
gSessionManager.Shutdown();
chip::DeviceLayer::PlatformMgr().Shutdown();
}
1 change: 1 addition & 0 deletions src/platform/tests/TestConfigurationMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ int TestConfigurationMgr_Setup(void * inContext)
*/
int TestConfigurationMgr_Teardown(void * inContext)
{
PlatformMgr().Shutdown();
chip::Platform::MemoryShutdown();
return SUCCESS;
}
Expand Down
Loading

0 comments on commit 371fb31

Please sign in to comment.