project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.35k stars 1.97k forks source link

DeviceProxy: Sort out ownership issues around the two proxy types #33031

Open cecille opened 5 months ago

cecille commented 5 months ago

Reproduction steps

Right now, OperationalDeviceProxy objects are minted in the callback to OnDeviceConnectedFn. This means there is a new proxy created each time the GetConnectedDevice is called, and the owner of the proxy is whatever called that function.

The CommissioneeDeviceProxy objects are owned by the DeviceCommissioner and reside in its pool. This is due to the early design of the controller, where the PASE session and commissioning were tightly tied, and the pase object (not a proxy at that point) never left the commissioner. When the pase object was moved over to the proxy model, the ownership model never changed. The result is that the pubic FindCommissioneeDevice function returns CommissioneeDeviceProxy objects directly from the DeviceCommissioner pool.

This is bad because as soon as these devolve into DeviceProxy objects, it becomes unclear how the ownership is handled.

One potential solution is to mint a new device proxy to return from FindCommissioneeDevice, but that's dangerous because there's existing code that might well rely on the Commissioner doing cleanup here. We want to make sure that we don't have dangling objects everywhere.

I'm not sure if there's a good solution to this problem at this point, especially given that there probably exists code OUTSIDE of the SDK that makes use of these proxies. Open to ideas here.

Bug prevalence

always

GitHub hash of the SDK that was being used

cb0751334e7ae5a7ad716c4f9bbc479e704d5369

Platform

core

Platform Version(s)

No response

Type

Core SDK Performance Improvement

Anything else?

No response

bzbarsky-apple commented 5 months ago

The right solution is probably to stop having DeviceProxy as a concept.

cecille commented 5 months ago

yeah, I kind of agree, but this is also now a public API. I'm not even sure we can cleanly deal with the ownership issues. It might just end up being a big ol' warning.