From d8d1756c40fd1ce1d48b31e36e1f1728e7c61238 Mon Sep 17 00:00:00 2001 From: James Swan <122404367+swan-amazon@users.noreply.github.com> Date: Wed, 5 Mar 2025 23:11:37 +0000 Subject: [PATCH] refactor: Rename CloseSession to MarkSessionDefunct and add MarkSessionForEviction This commit renames the Python function CloseSession to MarkSessionDefunct to better reflect its actual behavior and align with the C++ API. The function marks a session as temporarily defunct when there's a suspected loss of synchronization with the peer, but allows the session to become active again if messages are received. Additionally, adds a new function MarkSessionForEviction that permanently marks a session for removal by detaching all SessionHolders and releasing references, which will deallocate the session if no references remain. Once marked for eviction, the session can never become active again. Both functions have been properly documented to explain their behavior, parameters, and differences. All references to CloseSession throughout the codebase have been updated to use MarkSessionDefunct. --- .../ChipDeviceController-ScriptBinding.cpp | 18 +++++- src/controller/python/chip/ChipDeviceCtrl.py | 58 ++++++++++++++++--- .../python/chip/commissioning/pase.py | 2 +- .../python/test/test_scripts/base.py | 12 ++-- src/python_testing/TC_CGEN_2_8.py | 2 +- src/python_testing/TC_CGEN_2_9.py | 2 +- src/python_testing/TC_ICDM_3_4.py | 2 +- .../mbed/integration_tests/common/utils.py | 2 +- .../integration-tests/common/utils.py | 2 +- 9 files changed, 79 insertions(+), 21 deletions(-) diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index b9ed46feeef15d..21d04d76a2d104 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -159,7 +159,8 @@ PyChipError pychip_DeviceController_SetCheckMatchingFabric(bool check); struct IcdRegistrationParameters; PyChipError pychip_DeviceController_SetIcdRegistrationParameters(bool enabled, const IcdRegistrationParameters * params); PyChipError pychip_DeviceController_ResetCommissioningParameters(); -PyChipError pychip_DeviceController_CloseSession(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid); +PyChipError pychip_DeviceController_MarkSessionDefunct(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid); +PyChipError pychip_DeviceController_MarkSessionForEviction(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid); PyChipError pychip_DeviceController_EstablishPASESessionIP(chip::Controller::DeviceCommissioner * devCtrl, const char * peerAddrStr, uint32_t setupPINCode, chip::NodeId nodeid, uint16_t port); PyChipError pychip_DeviceController_EstablishPASESessionBLE(chip::Controller::DeviceCommissioner * devCtrl, uint32_t setupPINCode, @@ -661,7 +662,7 @@ PyChipError pychip_DeviceController_ResetCommissioningParameters() return ToPyChipError(CHIP_NO_ERROR); } -PyChipError pychip_DeviceController_CloseSession(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid) +PyChipError pychip_DeviceController_MarkSessionDefunct(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid) { // // Since we permit multiple controllers per fabric and each is associated with a unique fabric index, closing a session @@ -678,6 +679,19 @@ PyChipError pychip_DeviceController_CloseSession(chip::Controller::DeviceCommiss return ToPyChipError(CHIP_NO_ERROR); } +PyChipError pychip_DeviceController_MarkSessionForEviction(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid) +{ + devCtrl->SessionMgr()->ForEachMatchingSessionOnLogicalFabric(ScopedNodeId(nodeid, devCtrl->GetFabricIndex()), + [](auto * session) { + if (session->IsActiveSession()) + { + session->MarkForEviction(); + } + }); + + return ToPyChipError(CHIP_NO_ERROR); +} + PyChipError pychip_DeviceController_EstablishPASESessionIP(chip::Controller::DeviceCommissioner * devCtrl, const char * peerAddrStr, uint32_t setupPINCode, chip::NodeId nodeid, uint16_t port) { diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 1dac207b0f3111..b6c55aa36b1375 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -1,5 +1,5 @@ # -# Copyright (c) 2020-2022 Project CHIP Authors +# Copyright (c) 2020-2025 Project CHIP Authors # Copyright (c) 2019-2020 Google, LLC. # Copyright (c) 2013-2018 Nest Labs, Inc. # All rights reserved. @@ -702,12 +702,52 @@ def ExpireSessions(self, nodeid): self._ChipStack.Call(lambda: self._dmLib.pychip_ExpireSessions(self.devCtrl, nodeid)).raise_on_error() - # TODO: This needs to be called MarkSessionDefunct - def CloseSession(self, nodeid): + def MarkSessionDefunct(self, nodeid): + ''' + Marks a previously active session with the specified node as defunct to temporarily prevent it + from being used with new exchanges to send or receive messages. This should be called when there + is suspicion of a loss-of-sync with the session state on the associated peer, such as evidence + of transport failure. + + If messages are received thereafter on this session, the session will be put back into the Active state. + + This function should only be called on an active session. + This will NOT detach any existing SessionHolders. + + Parameters: + nodeid (int): The node ID of the device whose session should be marked as defunct. + + Raises: + RuntimeError: If the controller is not active. + PyChipError: If the operation fails. + ''' + self.CheckIsActive() + + self._ChipStack.Call( + lambda: self._dmLib.pychip_DeviceController_MarkSessionDefunct( + self.devCtrl, nodeid) + ).raise_on_error() + + def MarkSessionForEviction(self, nodeid): + ''' + Marks the session with the specified node for eviction. It will first detach all SessionHolders + attached to this session by calling 'OnSessionReleased' on each of them. This will force them + to release their reference to the session. If there are no more references left, the session + will then be de-allocated. + + Once marked for eviction, the session SHALL NOT ever become active again. + + Parameters: + nodeid (int): The node ID of the device whose session should be marked for eviction. + + Raises: + RuntimeError: If the controller is not active. + PyChipError: If the operation fails. + ''' self.CheckIsActive() self._ChipStack.Call( - lambda: self._dmLib.pychip_DeviceController_CloseSession( + lambda: self._dmLib.pychip_DeviceController_MarkSessionForEviction( self.devCtrl, nodeid) ).raise_on_error() @@ -1873,9 +1913,13 @@ def _InitLib(self): c_void_p, c_uint64, _DeviceUnpairingCompleteFunct] self._dmLib.pychip_DeviceController_UnpairDevice.restype = PyChipError - self._dmLib.pychip_DeviceController_CloseSession.argtypes = [ + self._dmLib.pychip_DeviceController_MarkSessionDefunct.argtypes = [ c_void_p, c_uint64] - self._dmLib.pychip_DeviceController_CloseSession.restype = PyChipError + self._dmLib.pychip_DeviceController_MarkSessionDefunct.restype = PyChipError + + self._dmLib.pychip_DeviceController_MarkSessionForEviction.argtypes = [ + c_void_p, c_uint64] + self._dmLib.pychip_DeviceController_MarkSessionForEviction.restype = PyChipError self._dmLib.pychip_DeviceController_GetAddressAndPort.argtypes = [ c_void_p, c_uint64, c_char_p, c_uint64, POINTER(c_uint16)] @@ -2261,7 +2305,7 @@ async def CommissionOnNetwork(self, nodeId: int, setupPinCode: int, return await asyncio.futures.wrap_future(ctx.future) def get_rcac(self): - ''' + ''' Passes captured RCAC data back to Python test modules for validation - Setting buffer size to max size mentioned in spec: - Ref: https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/06c4d55962954546ecf093c221fe1dab57645028/policies/matter_certificate_policy.adoc#615-key-sizes diff --git a/src/controller/python/chip/commissioning/pase.py b/src/controller/python/chip/commissioning/pase.py index ee0b96c76baf73..61bce6f75d90cb 100644 --- a/src/controller/python/chip/commissioning/pase.py +++ b/src/controller/python/chip/commissioning/pase.py @@ -39,7 +39,7 @@ def __enter__(self) -> Session: device=self.devCtrl.GetConnectedDeviceSync(self.node_id, allowPASE=True, timeoutMs=1000)) def __exit__(self, type, value, traceback): - self.devCtrl.CloseSession(self.node_id) + self.devCtrl.MarkSessionDefunct(self.node_id) if self.is_ble: self.devCtrl.CloseBLEConnection(self.is_ble) diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index fb5b2e84e84529..204b2922ea406a 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/src/controller/python/test/test_scripts/base.py @@ -671,7 +671,7 @@ async def TestCaseEviction(self, nodeid: int): # the controller and target. # for x in range(minimumSupportedFabrics * minimumCASESessionsPerFabric * 2): - self.devCtrl.CloseSession(nodeid) + self.devCtrl.MarkSessionDefunct(nodeid) await self.devCtrl.ReadAttribute(nodeid, [(Clusters.BasicInformation.Attributes.ClusterRevision)]) self.logger.info("Testing CASE defunct logic") @@ -700,7 +700,7 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub # # This marks the session defunct. # - self.devCtrl.CloseSession(nodeid) + self.devCtrl.MarkSessionDefunct(nodeid) # # Now write the attribute from fabric2, give it some time before checking if the report @@ -732,7 +732,7 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub sub.SetAttributeUpdateCallback(OnValueChange) for x in range(minimumSupportedFabrics * minimumCASESessionsPerFabric * 2): - self.devCtrl2.CloseSession(nodeid) + self.devCtrl2.MarkSessionDefunct(nodeid) await self.devCtrl2.ReadAttribute(nodeid, [(Clusters.BasicInformation.Attributes.ClusterRevision)]) # @@ -760,7 +760,7 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub sub.SetAttributeUpdateCallback(OnValueChange) for x in range(minimumSupportedFabrics * minimumCASESessionsPerFabric * 2): - self.devCtrl.CloseSession(nodeid) + self.devCtrl.MarkSessionDefunct(nodeid) await self.devCtrl.ReadAttribute(nodeid, [(Clusters.BasicInformation.Attributes.ClusterRevision)]) await self.devCtrl.WriteAttribute(nodeid, [(1, Clusters.UnitTesting.Attributes.Int8u(6))]) @@ -1072,7 +1072,7 @@ async def OnResubscriptionSucceeded(transaction): def TestCloseSession(self, nodeid: int): self.logger.info(f"Closing sessions with device {nodeid}") try: - self.devCtrl.CloseSession(nodeid) + self.devCtrl.MarkSessionDefunct(nodeid) return True except Exception as ex: self.logger.exception( @@ -1476,7 +1476,7 @@ async def TestSubscriptionResumptionCapacityStep2(self, nodeid: int, endpoint: i self.logger.info("Send a new subscription request from the second controller") # Close previous session so that the second controller will res-establish the session with the remote device - self.devCtrl.CloseSession(nodeid) + self.devCtrl.MarkSessionDefunct(nodeid) await self.devCtrl.ReadAttribute(nodeid, [(endpoint, Clusters.BasicInformation.Attributes.NodeLabel)], None, False, reportInterval=(1, 50), keepSubscriptions=True, autoResubscribe=False) diff --git a/src/python_testing/TC_CGEN_2_8.py b/src/python_testing/TC_CGEN_2_8.py index 5e154cd15ee719..abe94393b4500e 100644 --- a/src/python_testing/TC_CGEN_2_8.py +++ b/src/python_testing/TC_CGEN_2_8.py @@ -130,7 +130,7 @@ async def test_TC_CGEN_2_8(self): ) # Close the commissioner session with the device to clean up resources - commissioner.CloseSession(nodeid=self.dut_node_id) + commissioner.MarkSessionDefunct(nodeid=self.dut_node_id) # Step 5: Factory reset is handled by test operator self.step(5) diff --git a/src/python_testing/TC_CGEN_2_9.py b/src/python_testing/TC_CGEN_2_9.py index fce3b5a2a9b4e3..cc2cbecaa12e3b 100644 --- a/src/python_testing/TC_CGEN_2_9.py +++ b/src/python_testing/TC_CGEN_2_9.py @@ -158,7 +158,7 @@ async def test_TC_CGEN_2_9(self): await self.remove_commissioner_fabric() # Close the commissioner session with the device to clean up resources - commissioner.CloseSession(nodeid=self.dut_node_id) + commissioner.MarkSessionDefunct(nodeid=self.dut_node_id) # Step 6: Put device in commissioning mode (requiring user input, so skip in CI) self.step(6) diff --git a/src/python_testing/TC_ICDM_3_4.py b/src/python_testing/TC_ICDM_3_4.py index 8ff2a570376505..68bee266984859 100644 --- a/src/python_testing/TC_ICDM_3_4.py +++ b/src/python_testing/TC_ICDM_3_4.py @@ -128,7 +128,7 @@ async def test_TC_ICDM_3_4(self): paaTrustStorePath=str(self.config.paa_trust_store_path), catTags=self.config.controller_cat_tags ) - devCtrl.CloseSession(self.dut_node_id) + devCtrl.MarkSessionDefunct(self.dut_node_id) icdCounter2 = await self._read_icdm_attribute_expect_success(attribute=attributes.ICDCounter) asserts.assert_greater_equal(icdCounter2, icdCounter1, "ICDCounter have reboot is not greater or equal to the ICDCounter read before the reboot.") diff --git a/src/test_driver/mbed/integration_tests/common/utils.py b/src/test_driver/mbed/integration_tests/common/utils.py index 2b1db4da30ab8e..36cbdf5e652912 100644 --- a/src/test_driver/mbed/integration_tests/common/utils.py +++ b/src/test_driver/mbed/integration_tests/common/utils.py @@ -235,7 +235,7 @@ def close_connection(devCtrl, nodeId): :return: true if successful, otherwise false """ try: - devCtrl.CloseSession(nodeId) + devCtrl.MarkSessionDefunct(nodeId) except exceptions.ChipStackException as ex: log.error("Close session failed: {}".format(str(ex))) return False diff --git a/src/test_driver/openiotsdk/integration-tests/common/utils.py b/src/test_driver/openiotsdk/integration-tests/common/utils.py index da2dcff787de7a..9c75d5331d0a7d 100644 --- a/src/test_driver/openiotsdk/integration-tests/common/utils.py +++ b/src/test_driver/openiotsdk/integration-tests/common/utils.py @@ -108,7 +108,7 @@ def disconnect_device(devCtrl, nodeId): :return: node ID if connection successful or None if failed """ try: - devCtrl.CloseSession(nodeId) + devCtrl.MarkSessionDefunct(nodeId) except exceptions.ChipStackException as ex: log.error("CloseSession failed {}".format(str(ex))) return False