Fix some synchronization issues with RCTImageDownloader #117

arunv · 2015-03-03T03:44:53Z

Right now this uses an NSMutableDictionary with NSMutableArrays as objects to run the dependent blocks.

These are not threadsafe, and they cause 2 problems:

When enumerating the blocks to run with the results of a fetched image, if a new block shows up, we get an exception about concurrent access. You can repro this by scrolling down really fast on the UIExplorer's ListView
If the array of dependencies corresponding to a URL is set to nil, but another block managed to dequeue that array, it will add itself to the array but will never be executed. This means some blocks are going to just never run.

Additionally, there's a problem where only the first request for an image is able to be cancelled (the cancellation is handled through a local variable). This means that canceling one version of an image load could do nothing, or could cancel every other image request for the same image.

I changed this download to extend NSOperation, which supports dependencies that can queue up while the download is happening. It also allows us to remove individual operations from the list of dependencies.

Added a unittest to illustrate the issue.

vjeux · 2015-03-03T17:15:35Z

cc @a2 for review

sahrens · 2015-03-16T17:52:41Z

ping @a2, @nicklockwood

Use only one package for dependencies, for now. Fixes #225

…#282 #252

[react-packager] Fix assetRoots when starting in node_modules,

Support/use Promises

Before: > Command `foo` unrecognized.Did you mean to run this inside a react-native project? After: > Command `foo` unrecognized. Did you mean to run this inside a react-native project?

Adds missing space character to CLI error message

- [React Native] Sync from github | Amjad Masad - [react-packager] Watch asset roots for changes to update dependency graph | Amjad Masad - Fix sourceTree of RCTActionSheet.xcodeproj | Alex Kotliarskyi - Cancel contents animation before setting new contents in RCTNetworkImageView | Alex Akers - [react-packager] move dependencies to root package.json | Amjad Masad - Fix font crash on iOS < 8.2 | Nick Lockwood - [react-packager] Fix node v0.11.14 query parse bug | Amjad Masad - [ReactNative][Docs] Remove references to ReactNavigator from docs | Tadeu Zagallo - [CLI] react-native start won't run from dir with spaces | Amjad Masad - Revert .buckversion bumps. | Jakub Zika - [react_native] Update default bundle name to org.reactjs.native.* | Krzysztof Magiera - [react-packager] better error when main file not found | Amjad Masad - [React Kit] Remove embarrassing TODOs | Alex Akers - [ReactNative][MAdMan] Clean up after D1942269 | Philipp von Weitershausen - flowify a few more Libraries | Basil Hosmer - [ReactNative] PushNotificationIOS documentation | Eric Vicenti - [ReactNative][CustomComponents] Update old headers | Tadeu Zagallo - [ReactNative] UIViewControllerBasedStatusBarAppearance = NO in SampleApp | Alex Kotliarskyi - [React Native] Fix CocoaPods spec | Alex Akers - [ReactNative] Navigator Example Overhaul | Eric Vicenti - [React Native] Fix incorrect if-statement in RCTGeolocation | Alex Akers - [ReactNative] s/ReactKit/React/g | Tadeu Zagallo - [React Native] [FRC - Don't accept] View border support | Nick Lockwood - [Assets] Allow scripts to override assetRoots | Amjad Masad - [ReactNative] Navigator docs | Eric Vicenti - [ReactNative] License headers and renaming | Eric Vicenti - [React Native] Add CocoaPods spec | Tadeu Zagallo - Added explicit types for all view properties | Nick Lockwood - [ReactNative] s/ReactNavigator/Navigator/ | Tadeu Zagallo - [ReactNative] Add copyright header for code copied from the jQuery UI project | Martin Konicek - [ReactNative] PanResponder documentation | Eric Vicenti - [ReactNative] Add deep linking api | Tadeu Zagallo - [ReactNative] Add gitignore example for SampleApp | Alex Kotliarskyi - [ReactNative] Add react-native-start bin to react-native packge | Alex Kotliarskyi - [ReactNative] Update package.json to be npm-ready | Christopher Chedeau - [RFC][ReactNative] Integrate dev menu directly into RootView | Alex Kotliarskyi - flowify Libraries/ReactIOS | Marshall Roch - [WIP] Added support for italics and additional font weights | Nick Lockwood - [ReactNative] Improve View documentation | Christopher Chedeau - [react-packager] Readme | Amjad Masad - Fix for incorrect contentSize reported by RCTScrollView | Nick Lockwood - [ReactNative] Flow and doc formatting for NetInfo | Eric Vicenti - [ReactNative] Document AppStateIOS | Eric Vicenti - [MAdMan][Android] Make things look more Androidy | Philipp von Weitershausen - flowified Libraries from Avik | Basil Hosmer - flowify some Libraries | Basil Hosmer - [ReactKit] Add shake development menu | Alex Kotliarskyi - [ReactNative] Add debugger and change SampleApp files structure | Alex Kotliarskyi - Flowify ReactIOSEventEmitter | Marshall Roch - [react_native] JS files from D1941151: Allow fontWeight to be 100,200,...,900 | Krzysztof Magiera - [ReactNative] Add snapshot tests for examples | Spencer Ahrens - [ReactNative] bring back some native modules | Spencer Ahrens - [ReactNative] Rename JSNavigationStack to ReactNavigator, rename scene config | Eric Vicenti - [ReactNative] cleanup view example | Spencer Ahrens - Flowify a bunch of Libraries | Marshall Roch - [ReactNative] JSNavigationStack - Use key to blow away old scenes | Eric Vicenti - [ReactNative] Add more logging to RCTJSONParse | Sumeet Vaidya - Unfork UIManager | Nick Lockwood - [react-packager] kill non-standard RAW_SOURCE_MAP | Amjad Masad - Flowify Libraries/StyleSheet and Libraries/Text | Marshall Roch - [ReactNative] Fix OSS Dependency Issues | Eric Vicenti - [react-packager] Fix more issues with node modules | Amjad Masad - [ReactNative] rename navigationOperations to navigator | Eric Vicenti - JS files from D1936817: Add to XMLHttpRequest android and share code with ios | Olivia Bishop - flowify some Libraries | Basil Hosmer - last batch of UIExplorer flowification | Basil Hosmer - [ReactNative] JSNavigationStack rename routeMapper to renderSceneForRoute | Eric Vicenti - Flowify renderApplication | Marshall Roch - [ReactNative] OSS Responder example | Eric Vicenti - [ReactNative] Use oss TimerMixin | Tadeu Zagallo - [ReactNative] Remove auto permission request from setAppIconBadgeNumber | Tadeu Zagallo - [ReactNative] OSS snapshot tests | Spencer Ahrens - [ReactNative] OSS JSNavigationStack w/ Examples | Eric Vicenti - Fix build - remove relative import path | Jakub Zika - Bump .buckversion to a5b8b8ef45d714018ba3542cf98d48ef6aab7088. | Jakub Zika - [ReactNative] Open Source PushNotifications and move Badge Number methods and permission into it | Tadeu Zagallo - [react-packager] Fix regression with transform errors | Amjad Masad - Flowify TextStylePropTypes and fix a bug with unsupported props | Marshall Roch - [ReactNative] Remove `arc build` instructions from require | Alex Kotliarskyi - Flowify Library/Utilities/ | Marshall Roch - [react-packager] Default to index.js from main if it's a dir | Amjad Masad - [ReactNative] Use deprecated ix in TabBarExample | Amjad Masad - [ReactNative] Expanded license on obj-c files | Christopher Chedeau - [ReactNative] Expanded license on js files | Christopher Chedeau - [ReactNative] Fix React Devtools integration | Alex Kotliarskyi - [Text] Account for font leading so descenders are not clipped | Alex Kotliarskyi - [ReactNative] Expanded license on js packager files | Christopher Chedeau - more UIExplorer flow | Basil Hosmer - [react-packager] Pick up package changes while running | Amjad Masad - Added a graph view and a ReactNative metric that displays current queue and execution time for the JS thread. | Bryce Redd - [ReactNative] Add NativeModules and DeviceEventEmitter to react-native exports | Alex Kotliarskyi - [React Native] Fix iOS 7 crashes b/c missing Photos.fmwk | Alex Akers - UIExplorer flowification | Basil Hosmer - Add clearImmediate module | Marshall Roch - [ReactNative] Print directories packager is serving files from | Alex Kotliarskyi - Work around flow bug with exports | Marshall Roch - [ReactNative] Move packager/init.sh to GitHub | Alex Kotliarskyi - [ReactNative] Remove react-native/package.json | Christopher Chedeau - [ReactNative] Returning actual contentSize for RCTScrollViewManager | Henry Lung - declare timeoutID | Basil Hosmer - [ReactNative] Add root package.json name back | Tadeu Zagallo - [react-packager] Allow entry point extensions like .ios.js | Amjad Masad - [react-native] Use SpreadProperty to make react-docgen happy | Felix Kling - clean Examples/2048 | Basil Hosmer - [ReactNative] Adjust packager default root when running from within node_modules | Alex Kotliarskyi - [ReactNative] Add missing websocket dependency | Alex Kotliarskyi - [react-packager] change all but one `ix` to `require` | Amjad Masad - [react-packager] Make sure projectRoots is converted to an array | Amjad Masad - [ReactNative] Init script that bootstraps new Xcode project | Alex Kotliarskyi - [ReactNative] New SampleApp | Alex Kotliarskyi - [ReactNative] Touchable invoke press on longPress when longPress handler missing | Eric Vicenti - [ReactNative] Commit missing RCTWebSocketDebugger.xcodeproj | Alex Kotliarskyi

Update from Friday March 27

Fix 'respondible' typo in Tutorial

Update tutorial - use sample data

Autobuild website on Travis

…74731beabb3f9594d

Summary: facebook#117 identified some thread sync issues with our image downloader. I also noticed that we were decoding images on the main thread. This fixes both issues, and is a less radical change than proposed in the github PR. Test Plan: Run the Movies app and check images are popping in correctly.

Change tabs to spaces in todomvc example

* Verify if certs are needed for PR * Remove unused file

`assetType='All'` means that videos are also included which might cause a problem since the example only uses the `<Image />` element to display the photos

arunv mentioned this pull request Mar 3, 2015

RCTImageDownloader doesn't seem to be threadsafe #111

Closed

arasmussen force-pushed the master branch 5 times, most recently from 9b61be2 to 41453a5 Compare March 26, 2015 02:57

amasad and others added 22 commits March 26, 2015 17:26

Use only one package for dependencies, for now. Fixes #225

a0e09fe

Merge pull request #283 from facebook/update-packagejson

c15680a

Use only one package for dependencies, for now. Fixes #225

0.2.0

71f08d2

[react-packager] Fix assetRoots when starting in node_modules, fixes …

fc52ed6

…#282 #252

Merge pull request #286 from facebook/assetRoots

89fc8a4

[react-packager] Fix assetRoots when starting in node_modules,

0.2.1

bd8fa86

Fix log when execut npm start script

e9ef277

make a callback for server run only when done

10c2205

Merge pull request #290 from raphamorim/master

9c2dbbf

Support/use Promises

Adds missing space character to CLI error message

9095409

Before: > Command `foo` unrecognized.Did you mean to run this inside a react-native project? After: > Command `foo` unrecognized. Did you mean to run this inside a react-native project?

Merge pull request #307 from pgaspar/patch-1

3b7bd74

Adds missing space character to CLI error message

Fix 'respondible' typo in Tutorial

e23797b

Merge pull request #357 from facebook/updates-mar-27

0c83766

Update from Friday March 27

0.3.0

e02ea66

Merge pull request #359 from djkirby/fix-tutorial-typo

31c9843

Fix 'respondible' typo in Tutorial

Remove blocked rotten tomatoes api key

73bbef6

Update tutorial - use sample data

b00d4b3

Fix typo and add comments to the example app

9b0c2a4

Revert searches to SearchScreen.js - merging internally

bcb8c4c

Merge pull request #367 from tadeuzagallo/rotten-tomatoes

a8e3220

Update tutorial - use sample data

Merge pull request #381 from facebook/autopush-travis

7e9bbb6

Autobuild website on Travis

amasad added 2 commits March 27, 2015 22:11

Bring back accidentally reverted changes from 89fc8a4d7f9fa1c23ac235e…

c370d1a

…74731beabb3f9594d

0.3.1

3c43410

vjeux merged commit 3c43410 into facebook:master Mar 28, 2015

harrykiselev pushed a commit to harrykiselev/react-native that referenced this pull request Aug 5, 2015

Merge pull request facebook#117 from RileyTomasek/master

c36a9ac

Change tabs to spaces in todomvc example

acoates-ms added a commit to acoates-ms/react-native that referenced this pull request Jul 29, 2019

No longer need to install certs in PR (facebook#117)

4f86dc4

* Verify if certs are needed for PR * Remove unused file

jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020

Modify fontSize style to be able to animate fontSize (facebook#117)

a293beb

ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020

Corrected example (#117)

fe3b2bd

`assetType='All'` means that videos are also included which might cause a problem since the example only uses the `<Image />` element to display the photos

massapina mentioned this pull request Feb 23, 2021

[Snyk] Fix for 1 vulnerabilities massapina/react-native#23

Open

joseroubert08 mentioned this pull request Feb 24, 2021

[Snyk] Fix for 1 vulnerabilities joseroubert08/react-native#34

Open

admmasters mentioned this pull request Feb 24, 2021

[Snyk] Fix for 1 vulnerabilities admmasters/react-native#37

Open

harunpehlivan mentioned this pull request Feb 24, 2021

[Snyk] Fix for 1 vulnerabilities harunpehlivan/react-native#29

Open

enterstudio mentioned this pull request Feb 24, 2021

[Snyk] Fix for 1 vulnerabilities enterstudio/react-native#22

Open

AbhinavJain13 mentioned this pull request Feb 24, 2021

[Snyk] Fix for 3 vulnerabilities AbhinavJain13/react-native#24

Open

ebarahona mentioned this pull request Feb 24, 2021

[Snyk] Fix for 1 vulnerabilities ebarahona/react-native#48

Open

snyk-community mentioned this pull request Mar 1, 2021

[Snyk] Fix for 1 vulnerabilities snyk-community/react-native#24

Open

caschomburg123 mentioned this pull request Mar 1, 2021

[Snyk] Fix for 1 vulnerabilities caschomburg123/react-native#40

Open

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Fix for 16 vulnerabilities jrogatis/react-native#1

Open

ahmadzraiq mentioned this pull request Jul 22, 2022

[Android] Crash On Startup Using New Arch + Hermes #34237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some synchronization issues with RCTImageDownloader #117

Fix some synchronization issues with RCTImageDownloader #117

arunv commented Mar 3, 2015

vjeux commented Mar 3, 2015

sahrens commented Mar 16, 2015

Fix some synchronization issues with RCTImageDownloader #117

Fix some synchronization issues with RCTImageDownloader #117

Conversation

arunv commented Mar 3, 2015

vjeux commented Mar 3, 2015

sahrens commented Mar 16, 2015