Make OperationalDeviceProxy shareable by multiple applications, and delivers its callbacks to multiple applications

[kghost]

Problem

• 1: If an application context is released before the pairing completes, will render the pointer passed into OnDeviceConnected dangling
• 2: in OperationalDeviceProxy::DequeueConnectionCallbacks we may use member fields which is already destructed
  • cb->mCall(cb->mContext, mPeerId, error)
    • We are loop for all callbacks here, but this object may already destructed by previous callback, so mPeerId may not valid here
  • cb->mCall(cb->mContext, this)
    • Obviously, this may not valid here

Proposed Solution

• Implement a type-safe and RAII style callback queue using IntrusiveList to solve problem 1
• When implementing DequeueConnectionCallbacks, split the function into 2 parts:
  1. Move the callback queue to a stack variable
  2. Use an static member function to call all callback, to ensure that no member fields is used

[bzbarsky-apple]

The plan is to stop passing this to the callbacks altogether and just pass the session handle, right? So problem 1 is not worth worrying about, I suspect.

The use of mPeerId is a problem; very good catch there. Factoring out the callback-calling into a static function to avoid those problems seems good.

[kghost]

The plan is to stop passing this to the callbacks altogether and just pass the session handle, right? So problem 1 is not worth worrying about, I suspect.

I'm not clear how it resolves problem 1. The context pointer in the callback is dangling, calling the callback after its context is released will yield pretty bad result.

In DeviceCommissioner::OnDeviceConnectedFn, this (an OperationalDeviceProxy) is being used. So, problem 2.2 still need some attention.

I'm really frustrated dealing with object freeing itself inside its member function. can we just give OperationalDeviceProxy a reference counter and call it a day ?

[bzbarsky-apple]

The context pointer in the callback is dangling, calling the callback after its context is released will yield pretty bad result.

Anything that releases the context should Cancel() the callback, no? That's why we are using cancelables here.... I thought you meant the pointer to the OperationalDeviceProxy could dangle, for point 1.

In DeviceCommissioner::OnDeviceConnectedFn, this (an OperationalDeviceProxy) is being used. So, problem 2.2 still need some attention.

Problem 2.2 is what "just pass in the session instead" will solve, right?

I'm really frustrated dealing with object freeing itself inside its member function

Sure.

can we just give OperationalDeviceProxy a reference counter and call it a day ?

Again, the plan is to not have OperationalDeviceProxy at all in a way that is visible outside CASESessionManager....

Giving it a refcount would not really solve the issues here, by the way; who would be refcounting it and from where?

[kghost]

@bzbarsky-apple When trying to solve problem 2.2, the OperationalDeviceProxy object is passed to DefaultOTARequestor::SendQueryImageRequest then to ClusterBase::Associate which assigned to ClusterBase::mDevice.

So I have to change mDevice to a session holder, this should be a straightforward and simple change, but not small and may impact memory usage. Also need to add a member of pointer to ExchangeManager to ClusterBase, which may impact memory usage.

[mrjerryjohns]

Just synced up with @kghost to clarify all of this:

1. Confirmed that Problem 1 isn't an issue with the existing Cancelable stuff. (side-note: It took as both some time to prove it was indeed dequeuing the item on destruction of the Cancelable...instrusive would have been so much easier to deduce that...)
2. Went over the model that me and @bzbarsky-apple had previously agreed to, where-by CASESessionManager manages the allocation AND de-allocation of proxy instances. OnDeviceConnectedFn's signature switches to just passing in a session handle, and a separate 'management' callback will be defined in OperationalDeviceProxy that contains an OnDone(OperationalDeviceProxy *proxy) virtual method that the manager will implement. This method will be invoked at the end of dequeuing all success/failure callbacks to trigger the free'ing up of proxy instances. @kghost is good with the approach.

[mrjerryjohns]

I'm labelling this 1.0 since this will become more imperative when we have multiple consumers of OperationalDeviceProxy in the SDK (IM will be one of them...)

[bzbarsky-apple]

@kghost Just to allay the ClusterBase concern: those are stack-only objects in practice.

[mrjerryjohns]

This is not completed. #19673 just unblocks the core work for this issue.

[mrjerryjohns]

As part of addressing this, we should also remove CASESessionManager::Release* APIs as well.

[woody-apple]

SVE/Cert Blocker Review: Does not appear to be blocking a test case, removing SVE.