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

Fetching specific JSON causes massive memory leak #20352

Closed
Ashoat opened this issue Jul 23, 2018 · 20 comments
Closed

Fetching specific JSON causes massive memory leak #20352

Ashoat opened this issue Jul 23, 2018 · 20 comments
Labels
Bug Contributor A React Native contributor. Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. JavaScript 🌐Networking Related to a networking API. Partner Platform: iOS iOS applications. Priority: Mid Resolution: Locked This issue was locked by the bot.

Comments

@Ashoat
Copy link
Contributor

Ashoat commented Jul 23, 2018

Environment

React Native Environment Info:
  System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
    Memory: 6.28 GB / 32.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 10.6.0 - ~/.nvm/versions/node/v10.6.0/bin/node
    Yarn: 1.7.0 - /usr/local/bin/yarn
    npm: 6.1.0 - ~/.nvm/versions/node/v10.6.0/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 11.4, macOS 10.13, tvOS 11.4, watchOS 4.3
    Android SDK:
      Build Tools: 23.0.1, 25.0.0, 26.0.1, 26.0.2, 26.0.3, 28.0.1
      API Levels: 23, 26, 28
  IDEs:
    Android Studio: 3.1 AI-173.4819257
    Xcode: 9.4.1/9F2000 - /usr/bin/xcodebuild
  npmPackages:
    react: 16.4.1 => 16.4.1
    react-native: ^0.56.0 => 0.56.0
  npmGlobalPackages:
    react-native-git-upgrade: 0.2.7

Description

Downloading this <1 KiB JSON file once a second, parsing the JSON, and appending the resultant JSON to an array, causes the demo app below to hit 500MiB of memory usage in less than 15 seconds. Each request permanently allocates ~35MiB of memory.

Reproducible Demo

I have created a repo to reproduce the issue.

@Ashoat
Copy link
Contributor Author

Ashoat commented Jul 23, 2018

There are two issues going on here, in my judgement:

  1. The bigger issue, which is that a network request for a tiny JSON file is taking so much RAM.
  2. Network requests aren't being garbage collected if the resultant JSON isn't. If I let the resultant JSON get garbage collected, the memory leak doesn't occur. But the network request still needs to allocate a whole bunch of memory.

@kelset kelset added 🌐Networking Related to a networking API. JavaScript labels Jul 23, 2018
@Ashoat Ashoat mentioned this issue Jul 23, 2018
3 tasks
@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Jul 23, 2018
@hramos hramos added Core Team Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. labels Jul 23, 2018
@hramos hramos changed the title [iOS][Network] Fetching specific JSON causes massive memory leak Fetching specific JSON causes massive memory leak Jul 23, 2018
@corpsemxq
Copy link

corpsemxq commented Sep 12, 2018

Will this be fixed by #19333?
122b379

@Ashoat
Copy link
Contributor Author

Ashoat commented Sep 13, 2018

Unfortunately, no. I just updated the test repo to 0.57 and the issue is still occurring. After downloading the tiny JSON file once a second for sixty seconds (and not disposing of the result), memory usage of the Release build of the test project climbs to over 2 GiB.

@dlajarretie
Copy link

We're also experiencing the huge memory allocation when fetching JSON. We're fetching a much bigger resource than the example provided here, and I wonder if the memory allocation is proportional because our app gets killed by iOS for memory consumption (spike from 150mb to over 700mb), even before the fetch call completes.

@Ashoat
Copy link
Contributor Author

Ashoat commented Nov 12, 2018

I’ve been able to get around this issue by immediately cloning the JSON response after receiving it.

@alainib
Copy link

alainib commented Dec 19, 2018

i get a similar problem. i fetch a 15mo Json file. often the app get killed or get stuck in loading it.

console.log(url); // curl this url take less than 5s to download it
try {
  let response = await fetch(url);

  console.log("response", response); // log ok 
  let statusCode = response.status;

  console.log(statusCode);   // log 200    
  if (statusCode == 200) {
    let data = await response.json();
    // data = _.cloneDeep(data); // doesn't change anything

    console.log("json", data);  // never reach this log ( fast wifi connection ) !!!!!!!!!
  }
}	  

@zhongwuzw
Copy link
Contributor

Seems memory is fine in Release mode but bad in debug mode.

@dlajarretie
Copy link

Yes, in our case the memory consumption graph looks extremely different in debug mode compared to release.

@gablorquet
Copy link

Could it be related to this ?

@matt-oakes
Copy link
Contributor

@gablorquet Looking at the reproduction code, it doesn't look like NetInfo is involved.

@elicwhite
Copy link
Member

Does this repro on 0.59? Also, it isn't clear from the issue if this is iOS or Android. If it is Android I could imagine that this might be fixed by the latest JSC.

@zhongwuzw
Copy link
Contributor

zhongwuzw commented Mar 19, 2019

@TheSavior Yes, I confirmed on iOS, it has leaks, I checked both iOS and Android implementations, they share the same handle logic, so I assume Android may also has this issue. One leak we find is blob not removed, besides this, seems still has other leaks.

@TheSavior Oh, sorry, I messed up #23801 and this one, the comment above is based on #23801 .But I think this also has that problem.

@Ashoat
Copy link
Contributor Author

Ashoat commented Mar 19, 2019

If I recall correctly, this issue is iOS-only. My bad on not making that clear initially. The repro I made above is iOS-only.

I have updated the repro repo to RN 0.59.1 and unfortunately the issue remains.

@elicwhite
Copy link
Member

Got it. Thanks for giving it a try. Do you have any sense on whether this is the same problem as #23801? If so, as there is more conversation happening there than I'd like to close this as a duplicate.

@Ashoat
Copy link
Contributor Author

Ashoat commented Mar 19, 2019

Hmm... I took a quick look at the code to the repro in #23801, and on first glance I think they are different. But it's very possible they are caused by some of the same underlying issues.

Pulling from my comment above:

There are two issues going on here, in my judgement:

  1. The bigger issue, which is that a network request for a tiny JSON file is taking so much RAM.
  2. Network requests aren't being garbage collected if the resultant JSON isn't. If I let the resultant JSON get garbage collected, the memory leak doesn't occur. But the network request still needs to allocate a whole bunch of memory.

The first issue doesn't seem to apply in #23801, as the size of their downloaded file is pretty substantial (5 MiB) in contrast with the tiny file here (<1 KiB).

The second issue doesn't seem to apply either. In the repro for #23801, the author actually never keeps anything from the network request around, whereas I was only able to demonstrate my "massive" memory leak by keeping the resultant JSON blob and appending it to an array. My issue goes away if I simply clone the JSON blob and keep that instead of keeping what fetch returns to me.

It appears that there are two distinct memory leak issues: one that none of the native resources for the network request are cleaned up unless the resultant JS resources are garbage-collected, and the other that some of the native resources aren't cleaned up even if JS resources are never allocated.

@harleenarora
Copy link

harleenarora commented Mar 22, 2019

@Ashoat, Have you solved this problem? Do you have any solution for this.

facebook-github-bot pushed a commit that referenced this issue May 7, 2019
Summary:
Our Blob implementation was very problematic because it didn't release its underlying resource when the JS instance was dealocated. The main issue is that the fetch polyfill uses blobs by default if the module is available, which causes large memory leaks.

This fixes it by using the new jsi infra to attach a `jsi::HostObject` (`BlobCollector`)  to `Blob` instances. This way when the `Blob` is collected, the `BlobCollector` also gets collected. Using the `jsi::HostObject` dtor we can schedule the cleanup of native resources. This is definitely not the ideal solution but otherwise it would require rewriting the whole module using TurboModules + jsi.

Fixes #23801, #20352, #21092

[General] [Fixed] - [Blob] Release underlying resources when JS instance in GC'ed
Pull Request resolved: #24405

Differential Revision: D15237418

Pulled By: cpojer

fbshipit-source-id: 00a94a54b0b172fbc62324364b753d192ac7016a
facebook-github-bot pushed a commit that referenced this issue May 8, 2019
Summary:
Our Blob implementation was very problematic because it didn't release its underlying resource when the JS instance was dealocated. The main issue is that the fetch polyfill uses blobs by default if the module is available, which causes large memory leaks.

This fixes it by using the new jsi infra to attach a `jsi::HostObject` (`BlobCollector`)  to `Blob` instances. This way when the `Blob` is collected, the `BlobCollector` also gets collected. Using the `jsi::HostObject` dtor we can schedule the cleanup of native resources. This is definitely not the ideal solution but otherwise it would require rewriting the whole module using TurboModules + jsi.

Fixes #23801, #20352, #21092

[General] [Fixed] - [Blob] Release underlying resources when JS instance in GC'ed
Pull Request resolved: #24745

Reviewed By: fkgozali

Differential Revision: D15248848

Pulled By: hramos

fbshipit-source-id: 1da835cc935dfbf4e7bb6fbf2aea29bfdc9bd6fa
@janicduplessis
Copy link
Contributor

Fixed in 9ef5107 for ios and I just opened a pr with a fix for android.

@crobinson42
Copy link

Should this be re-opened? It's still an issue and as far as I understand the PR was pulled

@Ashoat
Copy link
Contributor Author

Ashoat commented Jun 18, 2019

I can't find evidence that the PR was pulled, but if you could demonstrate that the issue is still extant in 0.60 (which should include the PR) I think it would make sense to reopen. I can't seem to get react-native upgrade 0.60.0-rc.1 to work on the repo above, but maybe once 0.60 is released we can try upgrading it.

@jgreen210
Copy link

The fix on android is working for us with 0.61.5 (and presumably some earlier versions too, though we've not tried them). Only for the default JavaScriptCore/jsc engine though, not hermes or v8 (see the above two issues).

@facebook facebook locked as resolved and limited conversation to collaborators May 9, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Contributor A React Native contributor. Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. JavaScript 🌐Networking Related to a networking API. Partner Platform: iOS iOS applications. Priority: Mid Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests