[Python] Fix threading model (#20234) (#20465)

* Fix Python Threading Model

This fixes a number of issues with the Python REPL's threading model,
namely:

- Multiple, duplicated methods for stack initialization.
- Stack initialization split across really odd places
- Incorrect thread initialization
- Incorrect order of initialization

* Review feedback

* Brought back the auto-stack init in native, albeit, a minimal version
only.

Updated ChipStack to now use native to implicitly achieve this auto init
(which sets up MemoryInit) before continuining onwards with
controller-style initialization.

* Fix build

Co-authored-by: Jerry Johns <[email protected]>

src/controller/python/BUILD.gn

@@ -44,6 +44,8 @@ shared_library("ChipDeviceCtrl") {
     "chip/setup_payload/Parser.cpp",
   ]
 
+  sources += [ "chip/native/CommonStackInit.cpp" ]
+
   if (chip_controller) {
     sources += [
       "ChipCommissionableNodeController-ScriptBinding.cpp",
@@ -63,7 +65,6 @@ shared_library("ChipDeviceCtrl") {
       "chip/internal/ChipThreadWork.h",
       "chip/internal/CommissionerImpl.cpp",
       "chip/logging/LoggingRedirect.cpp",
-      "chip/native/StackInit.cpp",
     ]
   } else {
     sources += [

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -40,12 +40,6 @@
 #include <inttypes.h>
 #include <net/if.h>
 
-#include "ChipDeviceController-ScriptDevicePairingDelegate.h"
-#include "ChipDeviceController-StorageDelegate.h"
-
-#include "chip/interaction_model/Delegate.h"
-
-#include <app/CommandSender.h>
 #include <app/DeviceProxy.h>
 #include <app/InteractionModelEngine.h>
 #include <app/server/Dnssd.h>
@@ -55,6 +49,11 @@
 #include <controller/CommissioningDelegate.h>
 #include <controller/CommissioningWindowOpener.h>
 #include <controller/ExampleOperationalCredentialsIssuer.h>
+
+#include <controller/python/ChipDeviceController-ScriptDevicePairingDelegate.h>
+#include <controller/python/ChipDeviceController-StorageDelegate.h>
+#include <controller/python/chip/interaction_model/Delegate.h>
+
 #include <credentials/GroupDataProviderImpl.h>
 #include <credentials/PersistentStorageOpCertStore.h>
 #include <credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h>
@@ -71,6 +70,10 @@
 #include <setup_payload/QRCodeSetupPayloadParser.h>
 #include <system/SystemClock.h>
 
+#include <platform/CommissionableDataProvider.h>
+#include <platform/PlatformManager.h>
+#include <platform/TestOnlyCommissionableDataProvider.h>
+
 using namespace chip;
 using namespace chip::Ble;
 using namespace chip::Controller;
@@ -106,7 +109,8 @@ chip::NodeId kDefaultLocalDeviceId = chip::kTestControllerNodeId;
 chip::NodeId kRemoteDeviceId       = chip::kTestDeviceNodeId;
 
 extern "C" {
-ChipError::StorageType pychip_DeviceController_StackInit();
+ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId);
+ChipError::StorageType pychip_DeviceController_StackShutdown();
 
 ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Controller::DeviceCommissioner ** outDevCtrl,
                                                                    chip::NodeId localDeviceId, bool useTestCommissioner);
@@ -176,8 +180,6 @@ ChipError::StorageType pychip_DeviceCommissioner_CloseBleConnection(chip::Contro
 uint8_t pychip_DeviceController_GetLogFilter();
 void pychip_DeviceController_SetLogFilter(uint8_t category);
 
-ChipError::StorageType pychip_Stack_Init();
-ChipError::StorageType pychip_Stack_Shutdown();
 const char * pychip_Stack_ErrorToString(ChipError::StorageType err);
 const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t statusCode);
 void pychip_Stack_SetLogFunct(LogMessageFunct logFunct);
@@ -187,8 +189,6 @@ ChipError::StorageType pychip_GetConnectedDeviceByNodeId(chip::Controller::Devic
 ChipError::StorageType pychip_GetDeviceBeingCommissioned(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId,
                                                          CommissioneeDeviceProxy ** proxy);
 uint64_t pychip_GetCommandSenderHandle(chip::DeviceProxy * device);
-// CHIP Stack objects
-ChipError::StorageType pychip_BLEMgrImpl_ConfigureBle(uint32_t bluetoothAdapterId);
 
 chip::ChipError::StorageType pychip_InteractionModel_ShutdownSubscription(SubscriptionId subscriptionId);
 
@@ -220,10 +220,17 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter()
     return sStorageAdapter;
 }
 
-ChipError::StorageType pychip_DeviceController_StackInit()
+ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId)
 {
     VerifyOrDie(sStorageAdapter != nullptr);
 
+#if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
+    // By default, Linux device is configured as a BLE peripheral while the controller needs a BLE central.
+    CHIP_ERROR err =
+        chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ bluetoothAdapterId, /* BLE central */ true);
+    VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
+#endif
+
     FactoryInitParams factoryParams;
     factoryParams.fabricIndependentStorage = sStorageAdapter;
 
@@ -236,6 +243,12 @@ ChipError::StorageType pychip_DeviceController_StackInit()
 
     factoryParams.enableServerInteractions = true;
 
+    // Hack needed due to the fact that DnsSd server uses the CommissionableDataProvider even
+    // when never starting commissionable advertising. This will not be used but prevents
+    // null pointer dereferences.
+    static chip::DeviceLayer::TestOnlyCommissionableDataProvider TestOnlyCommissionableDataProvider;
+    chip::DeviceLayer::SetCommissionableDataProvider(&TestOnlyCommissionableDataProvider);
+
     ReturnErrorOnFailure(DeviceControllerFactory::GetInstance().Init(factoryParams).AsInteger());
 
     //
@@ -249,6 +262,32 @@ ChipError::StorageType pychip_DeviceController_StackInit()
     //
     DeviceControllerFactory::GetInstance().RetainSystemState();
 
+    //
+    // Finally, start up the main Matter thread. Any further interactions with the stack
+    // will now need to happen on the Matter thread, OR protected with the stack lock.
+    //
+    ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().StartEventLoopTask().AsInteger());
+
+    return CHIP_NO_ERROR.AsInteger();
+}
+
+ChipError::StorageType pychip_DeviceController_StackShutdown()
+{
+    ChipLogError(Controller, "Shutting down the stack...");
+
+    //
+    // Let's stop the Matter thread, and wait till the event loop has stopped.
+    //
+    ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().StopEventLoopTask().AsInteger());
+
+    //
+    // There is the symmetric call to match the Retain called at stack initialization
+    // time. This will release all resources (if there are no other controllers active).
+    //
+    DeviceControllerFactory::GetInstance().ReleaseSystemState();
+
+    DeviceControllerFactory::GetInstance().Shutdown();
+
     return CHIP_NO_ERROR.AsInteger();
 }
 
@@ -555,38 +594,6 @@ ChipError::StorageType pychip_ScriptDevicePairingDelegate_SetCommissioningStatus
     return CHIP_NO_ERROR.AsInteger();
 }
 
-ChipError::StorageType pychip_Stack_Init()
-{
-    CHIP_ERROR err = CHIP_NO_ERROR;
-
-    err = chip::Platform::MemoryInit();
-    SuccessOrExit(err);
-
-#if !CHIP_SYSTEM_CONFIG_USE_SOCKETS
-
-    ExitNow(err = CHIP_ERROR_NOT_IMPLEMENTED);
-
-#else /* CHIP_SYSTEM_CONFIG_USE_SOCKETS */
-
-#endif /* CHIP_SYSTEM_CONFIG_USE_SOCKETS */
-
-exit:
-    if (err != CHIP_NO_ERROR)
-        pychip_Stack_Shutdown();
-
-    return err.AsInteger();
-}
-
-ChipError::StorageType pychip_Stack_Shutdown()
-{
-    //
-    // There is the symmetric call to match the Retain called at stack initialization
-    // time.
-    //
-    DeviceControllerFactory::GetInstance().ReleaseSystemState();
-    return CHIP_NO_ERROR.AsInteger();
-}
-
 const char * pychip_Stack_ErrorToString(ChipError::StorageType err)
 {
     return chip::ErrorStr(CHIP_ERROR(err));

src/controller/python/chip/ChipStack.py

@@ -46,6 +46,8 @@
 from .clusters import Objects as GeneratedObjects
 from .clusters.CHIPClusters import *
 
+import chip.native
+
 __all__ = [
     "DeviceStatusStruct",
     "ChipStackException",
@@ -182,7 +184,10 @@ def __init__(self, persistentStoragePath: str, installDefaultLogHandler=True, bl
         self._activeLogFunct = None
         self.addModulePrefixToLogMessage = True
 
+        #
         # Locate and load the chip shared library.
+        # This also implictly does a minimal stack initialization (i.e call MemoryInit).
+        #
         self._loadLib()
 
         # Arrange to log output from the chip library to a python logger object with the
@@ -247,22 +252,17 @@ def HandleChipThreadRun(callback):
         # set by other modules(BLE) that require service by thread while thread blocks.
         self.blockingCB = None
 
-        # Initialize the chip library
-        res = self._ChipStackLib.pychip_Stack_Init()
-        if res != 0:
-            raise self.ErrorToException(res)
+        #
+        # Storage has to be initialized BEFORE initializing the stack, since the latter
+        # requires a PersistentStorageDelegate to be provided to DeviceControllerFactory.
+        #
+        self._persistentStorage = PersistentStorage(persistentStoragePath)
 
         if (bluetoothAdapter is None):
             bluetoothAdapter = 0
 
-        res = self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle(
-            bluetoothAdapter)
-        if res != 0:
-            raise self.ErrorToException(res)
-
-        self._persistentStorage = PersistentStorage(persistentStoragePath)
-
-        res = self._ChipStackLib.pychip_DeviceController_StackInit()
+        # Initialize the chip stack.
+        res = self._ChipStackLib.pychip_DeviceController_StackInit(bluetoothAdapter)
         if res != 0:
             raise self.ErrorToException(res)
 
@@ -321,7 +321,13 @@ def Shutdown(self):
         # Make sure PersistentStorage is destructed before chipStack
         # to avoid accessing builtins.chipStack after destruction.
         self._persistentStorage = None
-        self.Call(lambda: self._ChipStackLib.pychip_Stack_Shutdown())
+        self.Call(lambda: self._ChipStackLib.pychip_DeviceController_StackShutdown())
+
+        #
+        # Stack init happens in native, but shutdown happens here unfortunately.
+        # #20437 tracks consolidating these.
+        #
+        self._ChipStackLib.pychip_CommonStackShutdown()
         self.networkLock = None
         self.completeEvent = None
         self._ChipStackLib = None
@@ -409,42 +415,8 @@ def ErrorToException(self, err, devStatusPtr=None):
             )
 
     def LocateChipDLL(self):
-        if self._chipDLLPath:
-            return self._chipDLLPath
-
-        scriptDir = os.path.dirname(os.path.abspath(__file__))
-
-        # When properly installed in the chip package, the Chip Device Manager DLL will
-        # be located in the package root directory, along side the package's
-        # modules.
-        dmDLLPath = os.path.join(scriptDir, ChipStackDLLBaseName)
-        if os.path.exists(dmDLLPath):
-            self._chipDLLPath = dmDLLPath
-            return self._chipDLLPath
-
-        # For the convenience of developers, search the list of parent paths relative to the
-        # running script looking for an CHIP build directory containing the Chip Device
-        # Manager DLL. This makes it possible to import and use the ChipDeviceMgr module
-        # directly from a built copy of the CHIP source tree.
-        buildMachineGlob = "%s-*-%s*" % (platform.machine(),
-                                         platform.system().lower())
-        relDMDLLPathGlob = os.path.join(
-            "build",
-            buildMachineGlob,
-            "src/controller/python/.libs",
-            ChipStackDLLBaseName,
-        )
-        for dir in self._AllDirsToRoot(scriptDir):
-            dmDLLPathGlob = os.path.join(dir, relDMDLLPathGlob)
-            for dmDLLPath in glob.glob(dmDLLPathGlob):
-                if os.path.exists(dmDLLPath):
-                    self._chipDLLPath = dmDLLPath
-                    return self._chipDLLPath
-
-        raise Exception(
-            "Unable to locate Chip Device Manager DLL (%s); expected location: %s"
-            % (ChipStackDLLBaseName, scriptDir)
-        )
+        self._loadLib()
+        return self._chipDLLPath
 
     # ----- Private Members -----
     def _AllDirsToRoot(self, dir):
@@ -458,11 +430,13 @@ def _AllDirsToRoot(self, dir):
 
     def _loadLib(self):
         if self._ChipStackLib is None:
-            self._ChipStackLib = CDLL(self.LocateChipDLL())
-            self._ChipStackLib.pychip_Stack_Init.argtypes = []
-            self._ChipStackLib.pychip_Stack_Init.restype = c_uint32
-            self._ChipStackLib.pychip_Stack_Shutdown.argtypes = []
-            self._ChipStackLib.pychip_Stack_Shutdown.restype = c_uint32
+            self._ChipStackLib = chip.native.GetLibraryHandle()
+            self._chipDLLPath = chip.native.FindNativeLibraryPath()
+
+            self._ChipStackLib.pychip_DeviceController_StackInit.argtypes = [c_uint32]
+            self._ChipStackLib.pychip_DeviceController_StackInit.restype = c_uint32
+            self._ChipStackLib.pychip_DeviceController_StackShutdown.argtypes = []
+            self._ChipStackLib.pychip_DeviceController_StackShutdown.restype = c_uint32
             self._ChipStackLib.pychip_Stack_StatusReportToString.argtypes = [
                 c_uint32,
                 c_uint16,
@@ -474,10 +448,6 @@ def _loadLib(self):
                 _LogMessageFunct]
             self._ChipStackLib.pychip_Stack_SetLogFunct.restype = c_uint32
 
-            self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle.argtypes = [
-                c_uint32]
-            self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle.restype = c_uint32
-
             self._ChipStackLib.pychip_DeviceController_PostTaskOnChipThread.argtypes = [
                 _ChipThreadTaskRunnerFunct, py_object]
             self._ChipStackLib.pychip_DeviceController_PostTaskOnChipThread.restype = c_uint32

src/controller/python/chip/native/CommonStackInit.cpp

@@ -0,0 +1,61 @@
+/*
+ *
+ *    Copyright (c) 2020-2022 Project CHIP Authors
+ *    Copyright (c) 2019-2020 Google LLC.
+ *    Copyright (c) 2013-2018 Nest Labs, Inc.
+ *    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.
+ */
+
+/**
+ *    @file
+ *      Implementation of the native methods expected by the Python
+ *      version of Chip Device Manager.
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <system/SystemError.h>
+#include <system/SystemLayer.h>
+
+#include <lib/core/CHIPError.h>
+#include <lib/support/CHIPMem.h>
+#include <lib/support/CodeUtils.h>
+#include <lib/support/ScopedBuffer.h>
+#include <lib/support/logging/CHIPLogging.h>
+#include <platform/CHIPDeviceLayer.h>
+
+static_assert(std::is_same<uint32_t, chip::ChipError::StorageType>::value, "python assumes CHIP_ERROR maps to c_uint32");
+
+extern "C" {
+
+chip::ChipError::StorageType pychip_CommonStackInit()
+{
+    ReturnErrorOnFailure(chip::Platform::MemoryInit().AsInteger());
+    ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack().AsInteger());
+    return CHIP_NO_ERROR.AsInteger();
+}
+
+void pychip_CommonStackShutdown()
+{
+#if 0 //
+      // We cannot actually call this because the destructor for the MdnsContexts singleton on Darwin only gets called
+      // on termination of the program, and that unfortunately makes a bunch of Platform::MemoryFree calls.
+      //
+    chip::Platform::MemoryShutdown();
+#endif
+}
+};