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.

Sign up for GitHub

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

Jump to bottom

DeviceController shutdown should ShutdownSubscription on still-registered subscriptions. #22319

Closed
bzbarsky-apple opened this issue Aug 31, 2022 · 2 comments · Fixed by #22323
Closed

DeviceController shutdown should ShutdownSubscription on still-registered subscriptions. #22319

bzbarsky-apple opened this issue Aug 31, 2022 · 2 comments · Fixed by #22323
Assignees
@bzbarsky-apple
Labels
crash Interaction Model Work

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

During stack shutdown, if there are subscription ReadClients that are not waiting from a message from the server, or are but use auto-resubscribe, those ReadClients will not end up erroring out. That means the consumer will not be notified and will not know to destroy them, and if they are destroyed later they will crash as described in #20085

Proposed Solution

Since we know the ReadClients won't receive any more messages from the server (because we are removing them from the InteractionModelEngine list and because their resubscribe timers will never fire, since we are shutting down), we should just ShutdownSubscription on all of them. That will give API consumers a sane chance to clean them up.
@bzbarsky-apple
Copy link
Contributor Author

bzbarsky-apple commented Aug 31, 2022 •
edited
Loading

Alternately, we could do this from DeviceController:Shutdown....

@bzbarsky-apple
Copy link
Contributor Author

Actually, that last sounds best: shut down subscriptions for the relevant fabric index in DeviceController::Shutdown. I'll do that.

@bzbarsky-apple bzbarsky-apple self-assigned this Aug 31, 2022
@bzbarsky-apple bzbarsky-apple mentioned this issue Aug 31, 2022
Closed
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 31, 2022
@bzbarsky-apple
Shut down subscription clients when DeviceController shuts down.
e27e0ac 
Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes project-chip#22319
@bzbarsky-apple bzbarsky-apple mentioned this issue Aug 31, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Sep 1, 2022
@bzbarsky-apple
Shut down subscription clients when DeviceController shuts down.
e87751e 
Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes project-chip#22319
@bzbarsky-apple bzbarsky-apple changed the title Interaction model engine shutdown should ShutdownSubscription on still-registered subscriptions. DeviceController shutdown should ShutdownSubscription on still-registered subscriptions. Sep 1, 2022
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Sep 1, 2022
@bzbarsky-apple
Shut down subscription clients when DeviceController shuts down.
365fa9f 
Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes project-chip#22319
bzbarsky-apple added a commit that referenced this issue Sep 2, 2022
@bzbarsky-apple
Shut down subscription clients when DeviceController shuts down. (#22323
e56afce 
)

* Shut down subscription clients when DeviceController shuts down.

Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes #22319

* Suppress now-visible LSan leak and have IM engine shut down any remaining subscriptions.

* Fix lifetime management in subscribeAttributeWithEndpointId.

* Address review comment: reduce duplication in read client iteration methods.
isiu-apple pushed a commit to isiu-apple/connectedhomeip that referenced this issue Sep 16, 2022
@bzbarsky-apple @isiu-apple
Shut down subscription clients when DeviceController shuts down. (pro…
3bad65d 
…ject-chip#22323)

* Shut down subscription clients when DeviceController shuts down.

Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes project-chip#22319

* Suppress now-visible LSan leak and have IM engine shut down any remaining subscriptions.

* Fix lifetime management in subscribeAttributeWithEndpointId.

* Address review comment: reduce duplication in read client iteration methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
crash Interaction Model Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Shut down subscription clients when DeviceController shuts down. bzbarsky-apple/connectedhomeip
2 participants
@andy31415 @bzbarsky-apple