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.

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

Potential leak in ImagePipelineDelgate #807

Closed
prabhuamol opened this issue Aug 8, 2024 · 4 comments
Closed

Potential leak in ImagePipelineDelgate #807

prabhuamol opened this issue Aug 8, 2024 · 4 comments

Comments

@prabhuamol
Copy link

Hi
Should the ImagePipelineDelgate be declared as weak var. We are seeing a leak in the app regarding this.

We have ImagePipeline declared as a lazy var in a Class, this results in a retain cycle and a leak

   lazy var imagePipeline: ImagePipeline = {
        return .init(configuration: configuration, delegate: self)
   }()

I have a PR open here #806

Thanks in advance!

@prabhuamol
Copy link
Author

prabhuamol commented Aug 8, 2024

Looks like tests will fail as ImagePipelineDefaultDelgate wont hang around since its now weak. Hmm guess you cant have self as delegate like we are doing in order to avoid retain cycle

Should we be doing something like this

lazy var imagePipeline: ImagePipeline = {
        return .init(configuration: configuration, delegate: RemoteImagePipelineDelgate())
   }()

@kean
Copy link
Owner

kean commented Aug 8, 2024

The design matches URLSession where the delegate is also retained. It doesn't necessarily have to be, but it's more convenient for some scenarios.

@prabhuamol
Copy link
Author

prabhuamol commented Aug 8, 2024

Fair enough, we'll change our implementation accordingly. Maybe worth mentioning this in the doc as delegate are generally assumed to be weak

@prabhuamol
Copy link
Author

wonder if my other change here makes sense #806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants