Sort out who is responsible for cleaning up OperationalDeviceProxy instances when FindOrEstablishSession returns an error

[bzbarsky-apple]

Problem

Right now if someone calls CASESessionManager::FindOrEstablishSession and the result is a failure, it's not clear what happens to the OperationalDeviceProxy that was allocated to track the connection establishment. Some consumers call CASESessionManager::ReleaseSession on the peer id that failed to resolve. Some do nothing in particular.

Proposed Solution

The big question is whether it's worth holding on to proxies that have reached the HasAddress state even if the following CASE establishment failed. My suspicion is that it probably is not, especially in situations where there are OS-level DNS-SD caches.

If we accept that, and given that the failure callback does not provide a pointer to the proxy as part of its signature, I think we could have failed OperationalDeviceProxy instances automatically clean themselves up before dispatching the failure callbacks. They would need a pointer to the CASESessionManager to do that, I guess. But we could either add it, or replace the by-value DeviceProxyInitParams that a device proxy has now with a pointer to the CASESessionManager that it can then get the params from.

@kghost @msandstedt @mrjerryjohns thoughts?

[mrjerryjohns]

The big question is whether it's worth holding on to proxies that have reached the HasAddress state even if the following CASE establishment failed.

If we fail to establish CASE, starting again from a clean-slate and re-doing operational discovery is likely the best bet going forward. So I don't think holding onto that instance is worthwhile.

clean themselves up before dispatching the failure callbacks. They would need a pointer to the CASESessionManager to do that, I guess.

I'm not a big fan of objects deleting themselves, since it makes allocation and destruction quite asymmetric - I'd prefer the model where CASESessionManager handled both allocation and destruction if we could.

The suggestion I made in #18583 is one way to achieve that while preserving a large section of our current API surface.

[bzbarsky-apple]

I'm not a big fan of objects deleting themselves

To be clear, this would be more like a "I failed" call on the CASESessionManager, and that would then do the cleanup....

[kghost]

I agree upon the part that the address should not be handled inside OperationalDeviceProxy.

I'm not a big fan of objects deleting themselves

Agree, but we are already doing this in OperationalDeviceProxy::OnSessionEstablished/[Error], so at lease it doesn't make things worse. Whether we should follow #18583 is a separate story. (Actually, I prefer it than deleting OperationalDeviceProxy objects in themselves.)

[mrjerryjohns]

but we are already doing this in OperationalDeviceProxy::OnSessionEstablished

How so? It just invokes callbacks, it doesn't actually delete itself.

[mrjerryjohns]

So, the other detail here we need sorting out (which is the broader question here), is exactly how we expect multiple logical clients to interact with CASESessionManager to establish a session.

E.g The OTA-R cluster logic, as well as BindingsManager (see this comment in a recent PR).

This goes beyond just free'ing up on error...

[mrjerryjohns]

At some point, I had gotten to a good head-space in conversations with someone (forget whom), that every logical application client in the system would have their own instance of OperationalDeviceProxy. The underlying SecureSession would be shared by all, but each logical client would have their own proxy instance.

The pro with this model is that there is no need to then figure out shared-ptr-like allocation/de-allocation strategies for sharing use of OperationalDeviceProxy instances. It still would re-use the underlying CASE session if one had been established before, which is the other benefit.

The con with this model is that two clients setting up their own OperationalDeviceProxy instances at the same time would result in two CASE sessions being setup at the same time....

[bzbarsky-apple]

Right, sharing the discovery and session establishment is why we common up operational device proxies....

[bzbarsky-apple]

This might end up in significant API changes to how CASE is done.

[mrjerryjohns]

This is now duplicated by #19259