Skip to content

Commit

Permalink
[Python] Keep reference to callback function on timeout
Browse files Browse the repository at this point in the history
When using a timeout when calling GetConnectedDeviceSync() the callback
function DeviceAvailableCallback() gets potentially GC'ed. Make sure
we hold a reference to the instance.
  • Loading branch information
agners committed Dec 7, 2023
1 parent 4db8c38 commit 0281a60
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 17 deletions.
11 changes: 6 additions & 5 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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);
Expand Down Expand Up @@ -737,8 +737,8 @@ namespace {

struct GetDeviceCallbacks
{
GetDeviceCallbacks(DeviceAvailableFunc callback) :
mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mCallback(callback)
GetDeviceCallbacks(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)
Expand All @@ -758,15 +758,16 @@ struct GetDeviceCallbacks

Callback::Callback<OnDeviceConnected> mOnSuccess;
Callback::Callback<OnDeviceConnectionFailure> mOnFailure;
PyObject * const mContext;
DeviceAvailableFunc mCallback;
};
} // anonymous namespace

PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId,
DeviceAvailableFunc callback)
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));
}

Expand Down
29 changes: 17 additions & 12 deletions src/controller/python/chip/ChipDeviceCtrl.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
_DeviceAvailableFunct = 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)
Expand Down Expand Up @@ -759,25 +759,30 @@ 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)
if res.is_success:
print('Using PASE connection')
return DeviceProxyWrapper(returnDevice)

class DeviceAvailableClosure():
@_DeviceAvailableFunct
def DeviceAvailableCallback(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), 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.
Expand Down

0 comments on commit 0281a60

Please sign in to comment.