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 Map instead of object in BlobRegistry #39528

Conversation

ishikawa
Copy link
Contributor

issue: #39441

Summary:

For the following reasons, I have replaced an object used for id management inside BlobRegistry with Map.

To address this issue, I have modified the implementation of BlobRegistry to use a Map instead of an object. By using a Map, there is no limit to the number of entries.

Changelog:

[Internal] - [Fixed] - Fixed a bug that caused a "Property storage exceeds 196607 properties" error when sending a certain number of fetch requests.

Test Plan:

I've added a new tests in packages/react-native/Libraries/Blob/__tests__/BlobRegistry-test.js and confirmed the test pass before and after changes.

$ yarn run test
...
Test Suites: 1 skipped, 219 passed, 219 of 220 total
Tests:       2 skipped, 4017 passed, 4019 total
Snapshots:   1154 passed, 1154 total
Time:        10.525 s
Ran all test suites.
✨  Done in 12.52s.

@facebook-github-bot
Copy link
Contributor

Hi @ishikawa!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@github-actions
Copy link

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against 2e2823a

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2023
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 19, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 20, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in deca8c5.

@robsadams
Copy link

What version of react-native do we need to upgrade to in order to get this fix?

ShevO27 pushed a commit to ShevO27/react-native that referenced this pull request Sep 26, 2023
Summary:
issue: facebook#39441

For the following reasons, I have replaced an object used for id management inside BlobRegistry with `Map`.

- The polyfill used for `fetch`, [whatwg-fetch](https://github.com/JakeChampion/fetch), returns responses as `Blob` objects.
- When a `Blob` is created, it is registered with blobID in the [BlobRegistry](https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Blob/BlobRegistry.js), which is not automatically released.
- This issue was previously reported in facebook#19248 and was fixed by modifying whatwg-fetch. However, with the implementation of automatic garbage collection in facebook#24405, the implementation was reverted in commit bccc92d, returning to the original behavior.
- Although facebook#24405 enables `Blob` objects to be garbage collected, the Blob IDs registered in the BlobRegistry remain, causing the count to increase each time `fetch` is called.
- As a result, the `Property storage exceeds 196607 properties` error occurs

To address this issue, I have modified the implementation of `BlobRegistry` to use a `Map` instead of an object. By using a `Map`, there is no limit to the number of entries.

## Changelog:

[Internal] - [Fixed] - Fixed a bug that caused a "Property storage exceeds 196607 properties" error when sending a certain number of `fetch` requests.

Pull Request resolved: facebook#39528

Test Plan:
I've added a new tests in `packages/react-native/Libraries/Blob/__tests__/BlobRegistry-test.js` and confirmed the test pass before and after changes.

```
$ yarn run test
...
Test Suites: 1 skipped, 219 passed, 219 of 220 total
Tests:       2 skipped, 4017 passed, 4019 total
Snapshots:   1154 passed, 1154 total
Time:        10.525 s
Ran all test suites.
✨  Done in 12.52s.
```

Reviewed By: javache

Differential Revision: D49423213

Pulled By: NickGerleman

fbshipit-source-id: d5f73d7f5e34d1d2c3969b7dfbc45d3e6196aa30
@crackhack8266
Copy link

What version of react-native do we need to upgrade to in order to get this fix?

Did you get any solution as I am getting the same issue in react-native 0.72.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants