Memory leak on iOS

[owinter86]

Following up on the closed issue #325

This does seem to be an issue with memory leaks, which occur on app start.

Simple reproduction is init a new RN app ad add

React.useEffect(() => {
    const unsubscribe = NetInfo.addEventListener(() => {});
    return () => unsubscribe();
  }, []);

Environment

System:
    OS: macOS 10.15.7
    CPU: (12) x64 Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz
    Memory: 1.20 GB / 32.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 12.16.2 - ~/.nvm/versions/node/v12.16.2/bin/node
    Yarn: 1.22.5 - /usr/local/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v12.16.2/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.9.3 - /Users/oliverwinter/.rvm/gems/ruby-2.6.1/bin/pod
  SDKs:
    iOS SDK:
      Platforms: iOS 14.0, DriverKit 19.0, macOS 10.15, tvOS 14.0, watchOS 7.0
    Android SDK: Not Found
  IDEs:
    Android Studio: 4.0 AI-193.6911.18.40.6514223
    Xcode: 12.0.1/12A7300 - /usr/bin/xcodebuild
  Languages:
    Java: 1.8.0_242 - /usr/bin/javac
    Python: 2.7.16 - /usr/bin/python
  npmPackages:
    @react-native-community/cli: Not Found
    react: 16.13.1 => 16.13.1 
    react-native: 0.63.3 => 0.63.3 
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Platforms

Memory leaks were found in iOS on device iPhone 11 pro 14.0.1

Versions

"@react-native-community/netinfo": "5.9.7",

Description

Reproducible Demo

add effect in new app and profile on device to see leaks on app start

[owinter86]

I can confirm issue also exists when using the useNetInfo hook

const connected = useNetInfo();

[HessiPard]

Have this problem too.

[vcellu]

I am also seeing this as of version 6.0.0

[mikehardy]

Anyone have a PR to fix it? That would help

[chrisbobbe]

I'm not sure I'm reading this correctly, but:

ios/RNCNetInfo.m

- (instancetype)init
{
  self = [super init];
  if (self) {
    _connectionStateWatcher = [[RNCConnectionStateWatcher alloc] initWithDelegate:self];
  }
  return self;
}

That's the only place the identifier _connectionStateWatcher appears in the library, and I don't find anything relevant-looking outside the library in a Google search. Is this thing being used, and does it need teardown code after use? Or should it not be set in the first place?

Could the following, just below that, be a mistaken attempt to tear it down (connectionStateWatcher vs. _connectionStateWatcher)?

- (void)dealloc
{
  self.connectionStateWatcher = nil;
}

[ayubov]

The problem might crash an app. I'll try to use NetInfo.fetch() in interval, hope that will help

[ayubov]

Nope, looks like leaks are still there and these frequent checks are not good for performance

[mikehardy]

Auto release is all set up. Anyone with a PR can have a release almost immediately

[hypothete]

Any updates on this?

[mikehardy]

The beauty of open source is that if there are updates everyone can see them immediately. There's no secret updates. Anyone that wants to tackle this may feel free @hypothete ? 🙏

[alburdette619]

@mikehardy finally got some time to look into this and it is noted in the README, but still sets folks up for failure as most RN devs would expect more setup steps for native code or for it to be built into the library.

That being the link here to the Discussion section of the deprecated CNCopyCurrentNetworkInfo. I was about to put in a PR but not sure if it's appropriate as CNCopyCurrentNetworkInfo has other possibilities where it should work based off of that. My PR being to ensure we have location permissions to avoid the memory leak that not having them apparently causes.

What would be your advice on how to proceed or can you tag someone that can help out? I/someone could:

• Make the docs more explicit (you must get location permission, etc, here's how). The current copy requires a lot of a project outside of this library & imo it'd be better self contained as much as possible. Also, I didn't even look at the writeup on the SSID prop and still got a memory leak from it.
• I could put in the following change in the image. This would fix probably the biggest cases of this, but in its current iteration could cause others.
• Put in the PR & see how we could enhance. CNCopyCurrentNetworkInfo is deprecated for iOS 13+ & this library needs to implement the fetchCurrentWithCompletionHandler and again the following code probably isn't complete. Notably it takes a network state change after permissions are set on first run to then get SSID and the other possibilities for CNCopyCurrentNetworkInfo to function properly.

[alburdette619]

@mikehardy finally got some time to look into this and it is noted in the README, but still sets folks up for failure...

I didn't directly say this, but I profiled with and with out the above code and could directly see an effect on the memory leak on my own app, but had no issues on the example for some reason.

[alburdette619]

@matt-oakes ☝️? With the pettiness shown here I am trying to make sure I'm not being ignored seeing as responses otherwise came within a day. I have time to work on this but need direction, otherwise the potential for this open source contribution that @mikehardy is very adamant about, will devolve into just a patch for my own project.

[mikehardy]

Not sure about pettiness, I do my best to just state facts and I'm sorry if you feel my response is petty - not the intention, with regard to ignored, I've simply been really really busy and haven't had time. I've got this one queued though, a memory leak if demonstrated is a really big deal and I'd love to integrate any solution you've found

[mikehardy]

This response is taking longer because it now contains real work which requires real work to process 😅 - however, with the real work you've done I think it contains all the information required for anyone to verify it, which is a force multiplier in open source, whereas the initial fuzz-busting of "this doesn't have the info" is something I try to do really quickly just to make sure issues / reports meet minimum standards

[alburdette619]

...if demonstrated is a really big deal...

The leak was demonstrated by the initial writeup. You can see that CNCopyCurrentNetworkInfo is producing the leak & that it is from RNCNetInfo.

...I'd love to integrate any solution you've found

Do you have any input for the above writeup? I can just put in the PR & let it ride there if that'd be more useful. Clearly the library is not following the requirements for CNCopyCurrentNetworkInfo & putting that on the user. There are many resources online talking about this specifically causing leaks.

...with regard to ignored...

Apologies, with the speed and attitude of other replies I thought of the eventuality that you were simply going "ugh... just put in a PR, why are you commenting 🙄" or something of the like. Facts & how they are presented are not without meaning to others despite your intentions and I only have that to judge you from.

[mikehardy]

All fair, my presentation is certainly brief
I am not thinking "ugh just put in a PR", but I am thinking in general for all the repos I manage "please please someone just put in a PR and the associated testing across the compatibility matrix we support and documentation + release notes as needed so this is solved"

The absence of a PR means that work needs to be done and it all just takes time unfortunately

[matt-oakes]

🎉 This issue has been resolved in version 7.1.12 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀