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

Use proxy object to prevent strong reference cycle #55

Closed
wants to merge 1 commit into from

Conversation

flyinghyrax
Copy link
Contributor

This is a fix for the memory leak issue described in this comment on issue #12.

Although the deinit block called displayLink.invalidate(), because the CADisplayLink was retaining the AnimatableImageView instance the view was never deallocated, its deinit block never executed, and .invalidate() was never called.

We can fix the retention cycle between AnimatableImageView and CADisplayLink by using another object as the target for the CADisplayLink instance, as described here.

Here is the Instrument trace showing my test application running with this patch: gifu-leak-fixed.trace.zip. (Compare with the original trace from the comment linked above: gifu-leak-demo.trace.zip)

Screenshot of instrument results:
screen shot 2016-03-31 at 1 48 00 pm

@flyinghyrax flyinghyrax force-pushed the fix-retain-cycle branch 2 times, most recently from 08f7227 to 8ef09df Compare March 31, 2016 18:25
@stanmots
Copy link
Contributor

stanmots commented Apr 1, 2016

Well done! And just to add my 2 cents: I think, the only reason for using prepareForReuse method was due to this memory leak issue. Maybe it's better to mark it as deprecated now. For this purpose Swift provides the attribute:

@available(*, deprecated, message="This method will be removed in the next version!")

@kaishin
Copy link
Owner

kaishin commented Apr 10, 2016

Thank you @mr-seiler for the outstanding work. I am going over the PR and the demo project. I confirmed the retain cycle and will merge this shortly.

@kaishin kaishin closed this in 367144a Apr 10, 2016
@flyinghyrax
Copy link
Contributor Author

Glad to help 👍

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

Successfully merging this pull request may close these issues.

3 participants