From b375d848c0ea2999a7c0db0b681254accf27cc15 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Wed, 13 Dec 2023 19:51:20 +0100 Subject: [PATCH] [Python] Keep reference to callback function on timeout (#30877) * [Python] Keep reference to callback function on timeout When using a timeout when calling GetConnectedDeviceSync() the callback function DeviceAvailableCallback() gets potentially GC'ed. Make sure we hold a reference to the instance. * Use correct namespace for PyObject * Fix types in pychip_GetConnectedDeviceByNodeId * Call callback with context (self) * Correctly pass context * Use separate closure function --- .../ChipDeviceController-ScriptBinding.cpp | 17 ++++----- src/controller/python/chip/ChipDeviceCtrl.py | 35 ++++++++++++------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 2d6857d17a4fe6..3c0191072923b8 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -88,7 +88,7 @@ using namespace chip::DeviceLayer; extern "C" { typedef void (*ConstructBytesArrayFunct)(const uint8_t * dataBuf, uint32_t dataLen); typedef void (*LogMessageFunct)(uint64_t time, uint64_t timeUS, const char * moduleName, uint8_t category, const char * msg); -typedef void (*DeviceAvailableFunc)(DeviceProxy * device, PyChipError err); +typedef void (*DeviceAvailableFunc)(chip::Controller::Python::PyObject * context, DeviceProxy * device, PyChipError err); typedef void (*ChipThreadTaskRunnerFunct)(intptr_t context); typedef void (*DeviceUnpairingCompleteFunct)(uint64_t nodeId, PyChipError error); } @@ -207,7 +207,7 @@ const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t stat void pychip_Stack_SetLogFunct(LogMessageFunct logFunct); PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, - DeviceAvailableFunc callback); + chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback); PyChipError pychip_FreeOperationalDeviceProxy(chip::OperationalDeviceProxy * deviceProxy); PyChipError pychip_GetLocalSessionId(chip::OperationalDeviceProxy * deviceProxy, uint16_t * localSessionId); PyChipError pychip_GetNumSessionsToPeer(chip::OperationalDeviceProxy * deviceProxy, uint32_t * numSessions); @@ -737,36 +737,37 @@ namespace { struct GetDeviceCallbacks { - GetDeviceCallbacks(DeviceAvailableFunc callback) : - mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mCallback(callback) + GetDeviceCallbacks(chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback) : + mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mContext(context), mCallback(callback) {} static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle) { auto * self = static_cast(context); auto * operationalDeviceProxy = new OperationalDeviceProxy(&exchangeMgr, sessionHandle); - self->mCallback(operationalDeviceProxy, ToPyChipError(CHIP_NO_ERROR)); + self->mCallback(self->mContext, operationalDeviceProxy, ToPyChipError(CHIP_NO_ERROR)); delete self; } static void OnConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) { auto * self = static_cast(context); - self->mCallback(nullptr, ToPyChipError(error)); + self->mCallback(self->mContext, nullptr, ToPyChipError(error)); delete self; } Callback::Callback mOnSuccess; Callback::Callback mOnFailure; + chip::Controller::Python::PyObject * const mContext; DeviceAvailableFunc mCallback; }; } // anonymous namespace PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, - DeviceAvailableFunc callback) + chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback) { VerifyOrReturnError(devCtrl != nullptr, ToPyChipError(CHIP_ERROR_INVALID_ARGUMENT)); - auto * callbacks = new GetDeviceCallbacks(callback); + auto * callbacks = new GetDeviceCallbacks(context, callback); return ToPyChipError(devCtrl->GetConnectedDevice(nodeId, &callbacks->mOnSuccess, &callbacks->mOnFailure)); } diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 0eb22d5f81bf47..8f70c07c185fbf 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -76,7 +76,7 @@ # # CHIP_ERROR is actually signed, so using c_uint32 is weird, but everything # else seems to do it. -_DeviceAvailableFunct = CFUNCTYPE(None, c_void_p, PyChipError) +_DeviceAvailableCallbackFunct = CFUNCTYPE(None, py_object, c_void_p, PyChipError) _IssueNOCChainCallbackPythonCallbackFunct = CFUNCTYPE( None, py_object, PyChipError, c_void_p, c_size_t, c_void_p, c_size_t, c_void_p, c_size_t, c_void_p, c_size_t, c_uint64) @@ -100,6 +100,11 @@ class NOCChain: adminSubject: int +@_DeviceAvailableCallbackFunct +def _DeviceAvailableCallback(closure, device, err): + closure.deviceAvailable(device, err) + + @_IssueNOCChainCallbackPythonCallbackFunct def _IssueNOCChainCallbackPythonCallback(devCtrl, status: PyChipError, noc: c_void_p, nocLen: int, icac: c_void_p, icacLen: int, rcac: c_void_p, rcacLen: int, ipk: c_void_p, ipkLen: int, adminSubject: int): @@ -759,16 +764,6 @@ def GetConnectedDeviceSync(self, nodeid, allowPASE=True, timeoutMs: int = None): returnErr = None deviceAvailableCV = threading.Condition() - @_DeviceAvailableFunct - def DeviceAvailableCallback(device, err): - nonlocal returnDevice - nonlocal returnErr - nonlocal deviceAvailableCV - with deviceAvailableCV: - returnDevice = c_void_p(device) - returnErr = err - deviceAvailableCV.notify_all() - if allowPASE: res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned( self.devCtrl, nodeid, byref(returnDevice)), timeoutMs) @@ -776,8 +771,22 @@ def DeviceAvailableCallback(device, err): print('Using PASE connection') return DeviceProxyWrapper(returnDevice) + class DeviceAvailableClosure(): + def deviceAvailable(self, device, err): + nonlocal returnDevice + nonlocal returnErr + nonlocal deviceAvailableCV + with deviceAvailableCV: + returnDevice = c_void_p(device) + returnErr = err + deviceAvailableCV.notify_all() + ctypes.pythonapi.Py_DecRef(ctypes.py_object(self)) + + closure = DeviceAvailableClosure() + ctypes.pythonapi.Py_IncRef(ctypes.py_object(closure)) self._ChipStack.Call(lambda: self._dmLib.pychip_GetConnectedDeviceByNodeId( - self.devCtrl, nodeid, DeviceAvailableCallback), timeoutMs).raise_on_error() + self.devCtrl, nodeid, ctypes.py_object(closure), _DeviceAvailableCallback), + timeoutMs).raise_on_error() # The callback might have been received synchronously (during self._ChipStack.Call()). # Check if the device is already set before waiting for the callback. @@ -1568,7 +1577,7 @@ def _InitLib(self): self._dmLib.pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback.restype = PyChipError self._dmLib.pychip_GetConnectedDeviceByNodeId.argtypes = [ - c_void_p, c_uint64, _DeviceAvailableFunct] + c_void_p, c_uint64, py_object, _DeviceAvailableCallbackFunct] self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = PyChipError self._dmLib.pychip_FreeOperationalDeviceProxy.argtypes = [