Make cleanup functions return 'void' (#19870)

Destructors, Shutdown functions, and other cleanup code should be
declared with a 'void' return type as typically there is nothing that
can be done in the caller when an error is returned.

This removes the error return from such functions throughought Matter.
This transformation is straightforward in every case, and semantic
preserving in almost every case (broken semantics such as leaking memory
or leaving objects in an inconsistent state are not necessarily
preserved); it's rare for shutdown functions to actually return errors,
and also rare for callers to check for them.

Removing the threat of errors from cleanup functions simplifies control
flow in shutdown paths. It also facilitates shutdown from destructors
which is the typical C++ resource management paradigm ("RAII"), as
destructors have no return type.

Some functions had error cases that only occured when there was nothing
to do (e.g. not fully initialized returning 'invalid state'); these are
removed. The postcondition already holds in these cases, and maintaining
a precondition regarding the state would impose duplicative bookkeeping
in client code to track it without any real upside.

This changes a fair number of APIs, but the changes are easy to
accomodate as it is simply a matter of removing dead code (much of
which was already dead; one class even included the note

  @return #CHIP_NO_ERROR unconditionally.

as documentation that an error was not actually possible).

examples/chip-tool/commands/clusters/SubscriptionsCommands.h

@@ -56,7 +56,10 @@ class ShutdownSubscriptions : public CHIPCommand
     /////////// CHIPCommand Interface /////////
     CHIP_ERROR RunCommand() override
     {
-        CHIP_ERROR err = chip::app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex, mNodeId);
+        CHIP_ERROR err = CHIP_NO_ERROR;
+
+        chip::app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex, mNodeId);
+
         SetCommandExitStatus(err);
         return CHIP_NO_ERROR;
     }

examples/chip-tool/commands/common/CHIPCommand.cpp

@@ -153,33 +153,31 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CHIPCommand::MaybeTearDownStack()
+void CHIPCommand::MaybeTearDownStack()
 {
     if (IsInteractive())
     {
-        return CHIP_NO_ERROR;
+        return;
     }
 
     //
     // We can call DeviceController::Shutdown() safely without grabbing the stack lock
     // since the CHIP thread and event queue have been stopped, preventing any thread
     // races.
     //
-    ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityNull));
-    ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityAlpha));
-    ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityBeta));
-    ReturnLogErrorOnFailure(ShutdownCommissioner(kIdentityGamma));
+    ShutdownCommissioner(kIdentityNull);
+    ShutdownCommissioner(kIdentityAlpha);
+    ShutdownCommissioner(kIdentityBeta);
+    ShutdownCommissioner(kIdentityGamma);
 
     std::string name        = GetIdentity();
     chip::FabricId fabricId = strtoull(name.c_str(), nullptr, 0);
     if (fabricId >= kIdentityOtherFabricId)
     {
-        ReturnLogErrorOnFailure(ShutdownCommissioner(name));
+        ShutdownCommissioner(name);
     }
 
     StopTracing();
-
-    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CHIPCommand::Run()
@@ -201,7 +199,7 @@ CHIP_ERROR CHIPCommand::Run()
         Cleanup();
     }
 
-    ReturnErrorOnFailure(MaybeTearDownStack());
+    MaybeTearDownStack();
 
     return err;
 }
@@ -322,9 +320,9 @@ chip::Controller::DeviceCommissioner & CHIPCommand::GetCommissioner(const char *
     return *item->second;
 }
 
-CHIP_ERROR CHIPCommand::ShutdownCommissioner(std::string key)
+void CHIPCommand::ShutdownCommissioner(std::string key)
 {
-    return mCommissioners[key].get()->Shutdown();
+    mCommissioners[key].get()->Shutdown();
 }
 
 CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId fabricId,

examples/chip-tool/commands/common/CHIPCommand.h

@@ -139,11 +139,11 @@ class CHIPCommand : public Command
 
 private:
     CHIP_ERROR MaybeSetUpStack();
-    CHIP_ERROR MaybeTearDownStack();
+    void MaybeTearDownStack();
 
     CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricId fabricId,
                                       const chip::Credentials::AttestationTrustStore * trustStore);
-    CHIP_ERROR ShutdownCommissioner(std::string key);
+    void ShutdownCommissioner(std::string key);
     chip::FabricId CurrentCommissionerId();
     static std::map<std::string, std::unique_ptr<ChipDeviceCommissioner>> mCommissioners;
     static std::set<CHIPCommand *> sDeferredCleanups;

examples/darwin-framework-tool/commands/common/CHIPCommandBridge.h

@@ -94,7 +94,7 @@ class CHIPCommandBridge : public Command
 private:
     CHIP_ERROR InitializeCommissioner(std::string key, chip::FabricId fabricId,
                                       const chip::Credentials::AttestationTrustStore * trustStore);
-    CHIP_ERROR ShutdownCommissioner();
+    void ShutdownCommissioner();
     uint16_t CurrentCommissionerIndex();
 
     CHIP_ERROR mCommandExitStatus = CHIP_ERROR_INTERNAL;
@@ -103,7 +103,7 @@ class CHIPCommandBridge : public Command
     void StopWaiting();
 
     CHIP_ERROR MaybeSetUpStack();
-    CHIP_ERROR MaybeTearDownStack();
+    void MaybeTearDownStack();
 
     // Our three controllers: alpha, beta, gamma.
     static std::map<std::string, CHIPDeviceController *> mControllers;

examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm

@@ -47,7 +47,7 @@
     } else {
         Cleanup();
     }
-    ReturnErrorOnFailure(MaybeTearDownStack());
+    MaybeTearDownStack();
 
     return CHIP_NO_ERROR;
 }
@@ -105,14 +105,12 @@
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CHIPCommandBridge::MaybeTearDownStack()
+void CHIPCommandBridge::MaybeTearDownStack()
 {
-    CHIP_ERROR err;
     if (IsInteractive()) {
-        return CHIP_NO_ERROR;
+        return;
     }
-    err = ShutdownCommissioner();
-    return err;
+    ShutdownCommissioner();
 }
 
 void CHIPCommandBridge::SetIdentity(const char * identity)
@@ -130,7 +128,7 @@
 
 CHIPDeviceController * CHIPCommandBridge::GetCommissioner(const char * identity) { return mControllers[identity]; }
 
-CHIP_ERROR CHIPCommandBridge::ShutdownCommissioner()
+void CHIPCommandBridge::ShutdownCommissioner()
 {
     ChipLogProgress(chipTool, "Shutting down controller");
     for (auto & pair : mControllers) {
@@ -140,8 +138,6 @@
     mCurrentController = nil;
 
     [[MatterControllerFactory sharedInstance] shutdown];
-
-    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CHIPCommandBridge::StartWaiting(chip::System::Clock::Timeout duration)

examples/platform/linux/CommissionerMain.cpp

@@ -204,7 +204,7 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR ShutdownCommissioner()
+void ShutdownCommissioner()
 {
     UserDirectedCommissioningServer * udcServer = gCommissioner.GetUserDirectedCommissioningServer();
     if (udcServer != nullptr)
@@ -213,7 +213,6 @@ CHIP_ERROR ShutdownCommissioner()
     }
 
     gCommissioner.Shutdown();
-    return CHIP_NO_ERROR;
 }
 
 class PairingCommand : public Controller::DevicePairingDelegate

examples/platform/linux/CommissionerMain.h

@@ -35,7 +35,7 @@ CHIP_ERROR CommissionerPairOnNetwork(uint32_t pincode, uint16_t disc, PeerAddres
 CHIP_ERROR CommissionerPairUDC(uint32_t pincode, size_t index);
 
 CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort);
-CHIP_ERROR ShutdownCommissioner();
+void ShutdownCommissioner();
 
 DeviceCommissioner * GetDeviceCommissioner();
 CommissionerDiscoveryController * GetCommissionerDiscoveryController();

src/access/AccessControl.cpp

@@ -184,13 +184,12 @@ CHIP_ERROR AccessControl::Init(AccessControl::Delegate * delegate, DeviceTypeRes
     return retval;
 }
 
-CHIP_ERROR AccessControl::Finish()
+void AccessControl::Finish()
 {
-    VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturn(IsInitialized());
     ChipLogProgress(DataManagement, "AccessControl: finishing");
-    CHIP_ERROR retval = mDelegate->Finish();
-    mDelegate         = nullptr;
-    return retval;
+    mDelegate->Finish();
+    mDelegate = nullptr;
 }
 
 CHIP_ERROR AccessControl::CreateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t * index,

src/access/AccessControl.h

@@ -342,7 +342,7 @@ class AccessControl
         virtual void Release() {}
 
         virtual CHIP_ERROR Init() { return CHIP_NO_ERROR; }
-        virtual CHIP_ERROR Finish() { return CHIP_NO_ERROR; }
+        virtual void Finish() {}
 
         // Capabilities
         virtual CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const
@@ -430,7 +430,7 @@ class AccessControl
     /**
      * Deinitialize the access control module. Must be called when finished.
      */
-    CHIP_ERROR Finish();
+    void Finish();
 
     // Capabilities
     CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const

src/access/examples/ExampleAccessControlDelegate.cpp

@@ -965,11 +965,7 @@ class AccessControlDelegate : public AccessControl::Delegate
         return CHIP_NO_ERROR;
     }
 
-    CHIP_ERROR Finish() override
-    {
-        ChipLogProgress(DataManagement, "Examples::AccessControlDelegate::Finish");
-        return CHIP_NO_ERROR;
-    }
+    void Finish() override { ChipLogProgress(DataManagement, "Examples::AccessControlDelegate::Finish"); }
 
     CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const override
     {

src/access/examples/PermissiveAccessControlDelegate.cpp

@@ -31,7 +31,7 @@ class AccessControlDelegate : public AccessControl::Delegate
 {
 public:
     CHIP_ERROR Init() override { return CHIP_NO_ERROR; }
-    CHIP_ERROR Finish() override { return CHIP_NO_ERROR; }
+    void Finish() override {}
 
     // Capabilities
     CHIP_ERROR GetMaxEntryCount(size_t & value) const override

src/app/DeviceProxy.h

@@ -43,11 +43,11 @@ class DLL_EXPORT DeviceProxy
     /**
      *  Mark any open session with the device as expired.
      */
-    virtual CHIP_ERROR Disconnect() = 0;
+    virtual void Disconnect() = 0;
 
     virtual NodeId GetDeviceId() const = 0;
 
-    virtual CHIP_ERROR ShutdownSubscriptions() { return CHIP_ERROR_NOT_IMPLEMENTED; }
+    virtual void ShutdownSubscriptions() = 0;
 
     virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj, chip::Optional<System::Clock::Timeout> timeout = NullOptional);
 

src/app/InteractionModelEngine.cpp

@@ -231,7 +231,7 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscription(SubscriptionId aSubscrip
     return CHIP_ERROR_KEY_NOT_FOUND;
 }
 
-CHIP_ERROR InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId)
+void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId)
 {
     for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
     {
@@ -241,8 +241,6 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricInde
             readClient->Close(CHIP_NO_ERROR);
         }
     }
-
-    return CHIP_NO_ERROR;
 }
 
 void InteractionModelEngine::OnDone(CommandHandler & apCommandObj)

src/app/InteractionModelEngine.h

@@ -124,11 +124,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 
     /**
      * Tears down active subscriptions for a given peer node ID.
-     *
-     * @retval #CHIP_ERROR_KEY_NOT_FOUND If no active subscription is found.
-     * @retval #CHIP_NO_ERROR On success.
      */
-    CHIP_ERROR ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId);
+    void ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId);
 
     /**
      * Expire active transactions and release related objects for the given fabric index.
@@ -392,9 +389,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 
     bool HasActiveRead();
 
-    CHIP_ERROR ShutdownExistingSubscriptionsIfNeeded(Messaging::ExchangeContext * apExchangeContext,
-                                                     System::PacketBufferHandle && aPayload);
-
     inline size_t GetPathPoolCapacityForReads() const
     {
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST

src/app/OperationalDeviceProxy.cpp

@@ -313,9 +313,9 @@ void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session)
     // Do not touch this instance anymore; it might have been destroyed by a callback.
 }
 
-CHIP_ERROR OperationalDeviceProxy::Disconnect()
+void OperationalDeviceProxy::Disconnect()
 {
-    ReturnErrorCodeIf(mState != State::SecureConnected, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturn(mState == State::SecureConnected);
 
     if (mSecureSession)
     {
@@ -331,7 +331,6 @@ CHIP_ERROR OperationalDeviceProxy::Disconnect()
     mSecureSession.Release();
 
     MoveToState(State::HasAddress);
-    return CHIP_NO_ERROR;
 }
 
 void OperationalDeviceProxy::CleanupCASEClient()
@@ -358,9 +357,9 @@ void OperationalDeviceProxy::OnSessionHang()
     // TODO: establish a new session
 }
 
-CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
+void OperationalDeviceProxy::ShutdownSubscriptions()
 {
-    return app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex, GetDeviceId());
+    app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex, GetDeviceId());
 }
 
 OperationalDeviceProxy::~OperationalDeviceProxy()

src/app/OperationalDeviceProxy.h

@@ -167,13 +167,13 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
     /**
      *  Mark any open session with the device as expired.
      */
-    CHIP_ERROR Disconnect() override;
+    void Disconnect() override;
 
     NodeId GetDeviceId() const override { return mPeerId.GetNodeId(); }
 
     PeerId GetPeerId() const { return mPeerId; }
 
-    CHIP_ERROR ShutdownSubscriptions() override;
+    void ShutdownSubscriptions() override;
 
     Messaging::ExchangeManager * GetExchangeManager() const override { return mInitParams.exchangeMgr; }
 

src/app/clusters/network-commissioning/network-commissioning.cpp

@@ -70,10 +70,9 @@ CHIP_ERROR Instance::Init()
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR Instance::Shutdown()
+void Instance::Shutdown()
 {
-    ReturnErrorOnFailure(mpBaseDriver->Shutdown());
-    return CHIP_NO_ERROR;
+    mpBaseDriver->Shutdown();
 }
 
 void Instance::InvokeCommand(HandlerContext & ctxt)

src/app/clusters/network-commissioning/network-commissioning.h

@@ -46,7 +46,7 @@ class Instance : public CommandHandlerInterface,
      * Register will register the network commissioning instance to the attribute and command dispatching route.
      */
     CHIP_ERROR Init();
-    CHIP_ERROR Shutdown();
+    void Shutdown();
 
     // CommandHandlerInterface
     void InvokeCommand(HandlerContext & ctx) override;

src/app/server/Server.cpp

@@ -407,11 +407,7 @@ void Server::Shutdown()
     chip::Dnssd::Resolver::Instance().Shutdown();
     chip::app::InteractionModelEngine::GetInstance()->Shutdown();
     mMessageCounterManager.Shutdown();
-    CHIP_ERROR err = mExchangeMgr.Shutdown();
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(AppServer, "Exchange Mgr shutdown: %" CHIP_ERROR_FORMAT, err.Format());
-    }
+    mExchangeMgr.Shutdown();
     mSessions.Shutdown();
     mTransports.Close();
     mAccessControl.Finish();

src/app/tests/AppTestContext.cpp

@@ -49,14 +49,12 @@ CHIP_ERROR AppContext::Init()
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR AppContext::Shutdown()
+void AppContext::Shutdown()
 {
-    ReturnErrorOnFailure(Access::GetAccessControl().Finish());
+    Access::GetAccessControl().Finish();
 
     chip::app::InteractionModelEngine::GetInstance()->Shutdown();
-    ReturnErrorOnFailure(Super::Shutdown());
-
-    return CHIP_NO_ERROR;
+    Super::Shutdown();
 }
 
 } // namespace Test

src/app/tests/AppTestContext.h

@@ -33,7 +33,7 @@ class AppContext : public LoopbackMessagingContext
     CHIP_ERROR Init() override;
 
     // Shutdown all layers, finalize operations
-    CHIP_ERROR Shutdown() override;
+    void Shutdown() override;
 };
 
 } // namespace Test