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

POS: Fix retain cycles related to payment onboarding view #15083

Merged
merged 7 commits into from
Feb 7, 2025
Merged

POS: Fix retain cycles related to payment onboarding view #15083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d4e4eb
Break a strong reference cycle where onboardingViewModel.showSupport …
staskus Feb 6, 2025
74956ee
Capture POSAggregateModel in card connection Tasks weakly to release …
staskus Feb 6, 2025
42643d3
Merge branch 'trunk' into fix/14593-fix-pos-aggregate-modal-memory-le…
staskus Feb 6, 2025
434f8fe
Remove a retain cycle within CardPresentPaymentsOnboardingViewModel
staskus Feb 6, 2025
4b16658
Break a reference cycle between POSAggregateModel and CardPresentPaym…
staskus Feb 6, 2025
5c39bca
Don't hold CardPresentPaymentsOnboardingViewModel within CardPresentP…
staskus Feb 6, 2025
43c2942
Merge branch 'trunk' into fix/14593-fix-pos-aggregate-modal-memory-le…
staskus Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions WooCommerce/Classes/POS/Card Present Payments/CardPresentPaymentOnboardingAdaptor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ final class CardPresentPaymentsOnboardingPresenterAdaptor: CardPresentPaymentsOn

private let readinessUseCase: CardPresentPaymentsReadinessUseCase

private let onboardingViewModel: CardPresentPaymentsOnboardingViewModel
private var onboardingViewModel: CardPresentPaymentsOnboardingViewModel {
CardPresentPaymentsOnboardingViewModel(useCase: onboardingUseCase)
}

private var readinessSubscription: AnyCancellable?

Expand All @@ -20,7 +22,6 @@ final class CardPresentPaymentsOnboardingPresenterAdaptor: CardPresentPaymentsOn
init(stores: StoresManager = ServiceLocator.stores) {
onboardingUseCase = CardPresentPaymentsOnboardingUseCase(stores: stores)
readinessUseCase = CardPresentPaymentsReadinessUseCase(onboardingUseCase: onboardingUseCase, stores: stores)
onboardingViewModel = CardPresentPaymentsOnboardingViewModel(useCase: onboardingUseCase)
onboardingScreenViewModelPublisher = onboardingScreenViewModelSubject.eraseToAnyPublisher()
}

Expand Down
8 changes: 4 additions & 4 deletions WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,15 @@ extension PointOfSaleAggregateModel {

func connectCardReader() {
analytics.track(.pointOfSaleCardReaderConnectionTapped)
Task { @MainActor in
_ = try await cardPresentPaymentService.connectReader(using: .bluetooth)
Task { @MainActor [weak self] in
_ = try await self?.cardPresentPaymentService.connectReader(using: .bluetooth)
Comment on lines +163 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really form part of a retain cycle? We don't retain the Task, so I'm not sure why this is needed...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tasks are not released until the asynchronous work inside it finishes.

As I tested, attemptConnection never finishes when onboarding is opened and closed. Task is not released until the connectReader finishes and it doesn't finish. Since Task retains self through accessing cardPresentPaymentService, self (PointOfSaleAggregateModel) remains retained.

Task.weak.self.720p.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tasks are not released until the asynchronous work inside it finishes.

That's tricky to "visually" catch, as there are no explicit references to self. Thanks for the work on this and the explanation!

}
}

func disconnectCardReader() {
analytics.track(.cardReaderDisconnectTapped)
Task { @MainActor in
await cardPresentPaymentService.disconnectReader()
Task { @MainActor [weak self] in
await self?.cardPresentPaymentService.disconnectReader()
}
}

Expand Down
4 changes: 2 additions & 2 deletions WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ private extension PointOfSaleDashboardView {
}

func paymentsOnboardingView(from onboardingViewModel: CardPresentPaymentsOnboardingViewModel) -> some View {
onboardingViewModel.showSupport = {
posModel.cancelCardPaymentsOnboarding()
onboardingViewModel.showSupport = { [weak posModel] in
posModel?.cancelCardPaymentsOnboarding()
showSupport = true
}
return PointOfSaleCardPresentPaymentOnboardingView(viewModel: .init(onboardingViewModel: onboardingViewModel,
Expand Down
4 changes: 3 additions & 1 deletion ...elated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ final class CardPresentPaymentsOnboardingViewModel: ObservableObject, PaymentSet
self?.updateLearnMoreURL(state: result)
self?.reevaluateShouldShow(onboardingState: result)
})
.handleEvents(receiveOutput: trackState(_:))
.handleEvents(receiveOutput: { [weak self] state in
self?.trackState(state)
})
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great catch, it's not obvious at all that this captures self. I know it does, but it doesn't stand out.

.assign(to: &$state)
}

Expand Down
Loading