lngyeen / MySurveyChallenge

0 stars 0 forks source link

[Question] Concern with assign(to:, on:) #33

Closed blyscuit closed 4 months ago

blyscuit commented 4 months ago

Issue

lngyeen commented 4 months ago

@blyscuit

To be honest, I forgot to think about the memory leak issue. Well using assign(to: on:) can easily cause memory leak problem, in case of my LoginViewModel, .assign(to: .loginButtonEnabled, on: self) will cause a retain cycle (LoginViewModel hold cancellable from .assign(to: .loginButtonEnabled, on: self), in side var cancellables = Set(), and this cancellable strong hold LoginViewModel via .assign(to: .loginButtonEnabled, on: self),.

To avoid this problem I will switch to using the .sink() function and weak self pointer inside closure.

I also checked the loginUseCase.loginWith(email: username, password: password) function, which uses the sink function with a strong self pointer. I think self pointer should also be weak to avoid memory leaks. However, I did not detect a memory leak in this function. I think the reason is because the function loginUseCase.loginWith(email: username, password: password) will complete after receiving the response from the backend and the sink closure will be automatically released. But to be safe, weak pointer should be used. Please check this PR :) https://github.com/lngyeen/MySurveyChallenge/pull/46