Leak in RCTNetwork module

[yinghang]

• Review the documentation: https://facebook.github.io/react-native
• Search for existing issues: https://github.com/facebook/react-native/issues
• Use the latest React Native release: https://github.com/facebook/react-native/releases

Environment

Environment:
  OS: macOS High Sierra 10.13.5
  Node: 6.11.1
  Yarn: 1.6.0
  npm: 3.10.10
  Watchman: 4.9.0
  Xcode: Not Found
  Android Studio: 2.3 AI-162.4069837

Packages: (wanted => installed)
  react: 16.3.1 => 16.3.1
  react-native: 0.55.4 => 0.55.4

Description

A simple fetch call like the one below is enough to make various parts of RCTNetwork pile up references and memory allocations via malloc and etc. When I say pile up, what I mean is that every GET call is actually malloc-ing and retaining some data that never goes away. Simulating a low memory warning in Instruments also doesn't seem to get these references garbage collected away.

Reproducibility: 100%

  setInterval(() => {
    fetch('https://facebook.github.io/react-native/movies.json')
      .then((response) => response.json())
  }, 1000)

Screenshot 1

Screenshot 2

Screenshot 3

Screenshot 4

Some of the more obvious locations that I observed to be leaking:

• RCTNetworkTask.m - There seems to be a retain cycle in the completionBlock
• RCTNetworking.mm
• RCTHTTPRequestHandler.mm

TLDR: I'm not too familiar with the inner workings with RCTNetwork but the memory allocation seems to suggest that something funky is going on with the completionBlock which is leading to the retention of these references.

This is also a continuation of the conversation we had in #19169. @oNaiPs, you might be interested in this. 😄

Reproducible Demo

1. Clone this basic RN project @ https://github.com/yinghang/react-native-leak
2. Profile app with Instruments using Allocations
3. Watch RCTNetwork related references pile up

[atticoos]

cc a couple folks that show up in the blame for the networking modules
@javache @nicklockwood

[Ashoat]

Pretty sure I'm seeing this on RN0.56 as well, though for some reason it's not showing up in the Allocations tool mentioned in the issue. System Monitor shows the memory usage grow to 1400 MiB and the device log shows the kernel sending the process a SIGKILL due to the hard memory limit being reached. Once, the app crashed on an NSMallocException in RCTNetworkTask.m.

So far I've been able to largely alleviate the issue by getting rid of the Promise.race that wraps my fetch call. Not sure to what degree I've been successful yet.

[Ashoat]

I've figured out a workaround for my case. I'm using lodash's _cloneDeep to clone the response from fetch, which allows the original response to be garbage-collected. My release build is now stably using ~500MiB instead of leaking to 1400MiB in less than a minute.

Simplified version of my updated code:

const fetchResult = await fetch("http://domain.tld/path", input);
const text = await fetchResult.text();
const response = _cloneDeep(JSON.parse(text));

In my case, retaining the JSON.parse'd response prevented the entire network request from being garbage-collected. I was retaining most of the response because I build a Redux action out of it, and then cache the 20 most recent Redux actions in memory for debugging purposes. This normally doesn't take that much memory on the JS thread; the issue is the network requests not being garbage-collected.

I think the React Native issue I am seeing concretely is that as of 0.55, retaining a JSON.parse'd network response prevents the corresponding network request from being garbage-collected. This issue is extant as of 0.56.

I don't think this is the same issue as @yinghang initially reported, as their demo repo does not retain anything from the network request.

[Ashoat]

I've opened #20352 for my issue. It turns out downloading a specifically-formatted tiny JSON file can cause a much bigger memory leak.

Separately, I'm definitely able to confirm that OP's issue is still occurring in 0.56.0. The memory leak is much smaller, but still significant.

[corpsemxq]

Will this be fixed by #19333?
122b379

[yinghang]

Not too sure, will def give it a try when I have time.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.