Fix memory leaks in AppStateListener and Room#876
Fix memory leaks in AppStateListener and Room#876viachaslauinnowise wants to merge 1 commit intolivekit:mainfrom
Conversation
|
|
|
|
||
| // Stop listening to app state changes to avoid retaining Room longer than needed | ||
| await MainActor.run { | ||
| AppStateListener.shared.delegates.remove(delegate: self) |
There was a problem hiding this comment.
Can you explain why this is needed if AppStateListener uses:
private struct State {
let delegates = NSHashTable<AnyObject>.weakObjects()
}| } | ||
|
|
||
| deinit { | ||
| for (center, token) in _observerTokens { |
There was a problem hiding this comment.
This is a singleton, so where the removal will occur?
pblazej
left a comment
There was a problem hiding this comment.
As you see in the other PR(s) there's ongoing work to improve Swift concurrency and memory management in general, so thank you for bringing this topic.
However, this PR:
- does not provide any evidence that such leaks occur
- breaks reconnection semantics (as
AppStateListeneris added oninitbut removed duringcleanup()now) - abuses
deinitintroducing unsafeunowned
If we want to introduce some best practices here, we can try modernizing AppStateListener by leveraging Combine observers (AsyncStream is supported in iOS 15).
Otherwise, I'd suggest closing this PR.
Fix: memory leaks in AppStateListener observers and Room cleanup
AppStateListener:
Room: