Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We should not have a CASESessionManager per DeviceController #16174

Closed
bzbarsky-apple opened this issue Mar 14, 2022 · 2 comments · Fixed by #16233
Closed

We should not have a CASESessionManager per DeviceController #16174

bzbarsky-apple opened this issue Mar 14, 2022 · 2 comments · Fixed by #16233

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

CASESessionManager was changed to be fabric-independent, but we still have one per DeviceController (so one per fabric in the setup where we have DeviceControllers).

This causes a few problems:

  1. We can't easily implement the suggestion in Don't try to do something with message delivery failures on other fabrics. #16157 (comment) because we'd need to register all the CASESessionManagers as listeners for failed message delivery and because they are fabric-independent they would all try to restart sessions, unless we add some sort of filtering by session.
  2. We can't easily fix the broken interactions between CASEClient and CASESessionManager described in Establishing a CASE session tears down existing sessions for the peer #15986 because there are multiple CASESessionManagers floating around and we would need some way to find all of them.

Proposed Solution

Move the CASESessionManager into CHIPDeviceControllerSystemState and have DeviceController just point to it, not own it.

@bzbarsky-apple
Copy link
Contributor Author

Some specific issues regarding current members of DeviceController that are in the way of doing this:

  1. mCASEClientPool would need to move into CHIPDeviceControllerSystemState (or into CASESessionManager?)
  2. We'd need to sort out what happens with mIDAllocator. See also Multiple instances of session ID allocator create session ID collisions at shared Matter port #12821
  3. We'd need to sort out mDNSCache. Maybe that's getting removed, though.
  4. Would need to sort out what happens for mDevicePool.

@jtung-apple @msandstedt @mrjerryjohns

@bzbarsky-apple
Copy link
Contributor Author

bzbarsky-apple commented Mar 14, 2022

Maybe the answer for now is that mIDAllocator, mCASEClientPool, mDevicePool all move into the system state?

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Mar 15, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Mar 15, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Mar 15, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Mar 15, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Mar 15, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
bzbarsky-apple added a commit that referenced this issue Mar 18, 2022
We should have one CASESessionManager, not one per DeviceController.

Fixes #16174
ArekBalysNordic pushed a commit to ArekBalysNordic/connectedhomeip that referenced this issue Mar 18, 2022
…chip#16233)

We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
…chip#16233)

We should have one CASESessionManager, not one per DeviceController.

Fixes project-chip#16174
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant