-
Notifications
You must be signed in to change notification settings - Fork 33
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
IOS-8044 Fix multiple leaks #3940
Conversation
@@ -23,7 +23,9 @@ class CommonUserTokensManager { | |||
private let derivationStyle: DerivationStyle? | |||
private let existingCurves: [EllipticCurve] | |||
private let longHashesSupported: Bool | |||
private weak var keysDerivingProvider: KeysDerivingProvider? | |||
|
|||
weak var keysDerivingProvider: KeysDerivingProvider? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
техническая правка, чтобы завести фабрику
let userTokenListManager: UserTokenListManager | ||
let keysRepository: KeysRepository | ||
let derivationManager: DerivationManager? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommonUserWalletModel не деинитилась из-за derivationManager, который юзается в других сервисах, которые, в свою очередь лежали тут как lazy. Без изменения логики унес всю сборку из конструктора в отдельную фабрику, что в принципе чище, заодно и решило утечку
|
||
import Foundation | ||
|
||
struct CommonUserWalletModelFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По сути код без каких-либо изменений логики переехал сюда
} | ||
|
||
var body: some View { | ||
if #available(iOS 16, *) { | ||
ScrollView(.vertical, showsIndicators: false) { | ||
content | ||
} | ||
.refreshable { | ||
await refreshAsync() | ||
.refreshable { [weak refreshContainer] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Самая жесткая утечка, помог коммент @Vyachesl0ve. Теперь в памяти ничего не остается, это юзалось и в свопе, так что там тоже пофиксится, если конечно не найдется других утечек
@@ -81,6 +81,7 @@ final class MultiWalletMainContentViewModel: ObservableObject { | |||
private var isUpdating = false | |||
|
|||
private var bag = Set<AnyCancellable>() | |||
private var tokenListSyncSubscription: AnyCancellable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Утечка, из-за которой оставались в памяти TokenItemViewModel и бэленс провайдеры
tokenListSyncSubscription = Publishers.Zip(tokenListSyncPublisher, sectionsPublisher) | ||
.receive(on: DispatchQueue.main) | ||
.withWeakCaptureOf(self) | ||
.sink { viewModel, _ in | ||
viewModel.isLoadingTokenList = false | ||
withExtendedLifetime(tokenListSyncSubscription) {} | ||
viewModel.tokenListSyncSubscription = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я же правильно понял, что она одноразовая? @m3g0byt3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, пока не придут значения из обоих tokenListSyncPublisher
и sectionsPublisher
Здесь на самом деле можно было оставить старый код, просто добавить .prefix(1)
- никакой магии с withExtendedLifetime
нет, я просто не учел что это не one-time паблишеры
И в новом коде аналогично, можно .prefix(1)
вместо зануления подписки руками
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
поправил, проверил, все ок
guard let userWalletIdSeed = config.userWalletIdSeed, | ||
let walletManagerFactory = try? config.makeAnyWalletManagerFactory() else { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: как будто так чуток читабельнее
guard let userWalletIdSeed = config.userWalletIdSeed, | |
let walletManagerFactory = try? config.makeAnyWalletManagerFactory() else { | |
return nil | |
} | |
guard | |
let userWalletIdSeed = config.userWalletIdSeed, | |
let walletManagerFactory = try? config.makeAnyWalletManagerFactory() | |
else { | |
return nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Когда на это смотришь в гитхабе, то да) А в коде, с подсветкой синтаксиса кажется и так норм)
tokenListSyncSubscription = Publishers.Zip(tokenListSyncPublisher, sectionsPublisher) | ||
.receive(on: DispatchQueue.main) | ||
.withWeakCaptureOf(self) | ||
.sink { viewModel, _ in | ||
viewModel.isLoadingTokenList = false | ||
withExtendedLifetime(tokenListSyncSubscription) {} | ||
viewModel.tokenListSyncSubscription = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, пока не придут значения из обоих tokenListSyncPublisher
и sectionsPublisher
Здесь на самом деле можно было оставить старый код, просто добавить .prefix(1)
- никакой магии с withExtendedLifetime
нет, я просто не учел что это не one-time паблишеры
И в новом коде аналогично, можно .prefix(1)
вместо зануления подписки руками
60cfbe2
Починил несколько утечек памяти. Воспроизводилось легко - удалением одного из кошельков