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

Memory Leak on iOS: FastImage not released from memory when callbacks onLoad is set #371

Closed
StevenMasini opened this issue Dec 14, 2018 · 17 comments

Comments

@StevenMasini
Copy link

An engineer at my company noticed a memory leak on iOS when the props onLoad is set on iOS.
FastImage doesn't get released when it should be.

After investigating the source code, I found out that using a strong reference to self to call sendOnLoad in sd_setImageWithURL.

I suspect this line to be the cause of this memory leaks.

I can submit a Pull Request to transform this strong reference into a weak reference, which should fix the memory leak.

@oblador
Copy link

oblador commented Jan 27, 2019

Sounds like a good idea to me 🤷‍♂️

@StevenMasini
Copy link
Author

Ok sorry for the delay, I did the fix on my fork not long after opening this issue, but I forgot to submit the PR. Now it's done I submitted the PR.

@marf
Copy link

marf commented Feb 9, 2019

Hello @StevenMasini, I have tried your PR but it does not compile. (as I reported you in the PR)
What should I do?

Thank you,
Marco

@StevenMasini
Copy link
Author

StevenMasini commented Feb 20, 2019

@marf Hey sorry for the delay. I am working on it since yesterday. I found a very fast and easy way to reproduce the memory leak.

When you set the onError props in <FastImage>, but it's not really obvious yet what's causing this memory leak.

Also I am sorry for the fix I submitted earlier, it's been a while that I haven't done Objective C, so I forgot that typeof(self) will actually define the type as a pointer already.

So the correct syntax for the fix I submitted is __weak typeof(self) weakSelf = self; without the *.

But still, that's not enough to fix the memory leak. So I will keep investigating.

@LuongTruong
Copy link

HI @StevenMasini , if the onError is not be set, there is no memory leak right?

@StevenMasini
Copy link
Author

StevenMasini commented Mar 11, 2019

@truongluong1314520 You can set the onError but just don't try to get any parameters from it.

onError don't pass any parameters.

For instance do

onError={() => {
// your implemention
}}

but don't

onError={error => {
// this will create a memory leak
// the same for every callback who doesn't pass parameters, just strictly follow the documentation
// don't try to retrieve parameters who doesn't exist
console.log(error)
}}

@StevenMasini
Copy link
Author

I made a new Pull Request here #433

@StevenMasini
Copy link
Author

StevenMasini commented Mar 21, 2019

I would be happy if people could try my PR #433. It should fix the memory leak on iOS.

Let me know if it works for you or not ?

@marf
Copy link

marf commented Mar 23, 2019

Hello @StevenMasini I will try your PR #433 , thank you!
In my case I was not using the onError callback so I do not think that my problem is related to it.
I mitigated the problem by downsizing the images from 1024x1024 to 512x512, in these case with lots of images it takes less memory, but it is a workaround.

I will try your PR #433 hoping memory usage decreases.

Thank you

@StevenMasini
Copy link
Author

@marf Actually not only the onError callback is affected, but all callbacks. So even using onLoad will lead to memory leaks.

It's good to reduce the resolution of your pictures, you probably didn't do it for nothing.
I hope that the fix I made will solve your problem, if not I am willing to help you out.

@marf
Copy link

marf commented Mar 25, 2019

@StevenMasini thank you!

@manithin
Copy link

faced the same issue and had to remove the onError callback. Is there a fix coming for this?

@StevenMasini
Copy link
Author

@manithin What was your implementation of onError ? Did you try to retrieve a param from it ?

I remember that if you try to do this, it will lead to a memory leak.

onError={(error) => { ... }}

If so then just remove the param. onError does not return the specific error. If not, then that's another bug.

@manithin
Copy link

This is how i implement it. Just set an error status for displaying a default image.

onError={() => setImageError(true)}

when this block was disabled, there were immediate changes in the performance.

@kishanbharda
Copy link

I would be happy if people could try my PR #433. It should fix the memory leak on iOS.

Let me know if it works for you or not ?

Heyy...@StevenMasini I am also facing same crash issue on ios. I want to user you PR. But don't know how can I use your PR. I don't know much more about git. I just installed fastimage from npm. Can you please tell me how I can use it ?

@StevenMasini
Copy link
Author

@kishanbharda
The memory leaks fixes have been merged into the current version of react-native-fast-image.

But if you are experiencing issues with image loading, etc. My branch still fix more issues.

You can modify your package.json to point at my branch with the following line

"@stevenmasini/react-native-fast-image": "^7.0.0"

Also, I keep that branch up to date with the main repository.

@kishanbharda
Copy link

Thank you. I will try.

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

6 participants