Skip to content

Commit

Permalink
Avoid race condition for AppState.currentState
Browse files Browse the repository at this point in the history
Summary:
When the AppState module is initialized, it subscribes to the `appStateDidChange` event and sends an async native method call to the AppState native module. There is a small race condition window where the native module can read the current app state as `uninitialized` before calling the JavaScript callback, and then be interrupted by the underlying mechanism to trigger the `appStateDidChange` event. If the `appStateDidChange` event is processed before the JavaScript callback, the resulting value of `AppState.currentState` will be invalid.

Fixes microsoft/react-native-windows#1300

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

I simulated (over-exaggerated) the race condition by injecting "thread sleep" calls in the native method call and the native mechanism for updating the app state.

I then ran the AppStateExample in the RNTester and found that the current app state was set to `uninitialized`, as opposed to the expected value of `active`.

Once I made this JavaScript change, the over-exaggerated race condition no longer resulted in an invalid app state.
Closes #15499

Differential Revision: D5660620

Pulled By: hramos

fbshipit-source-id: 47c0dca75d37f677191c48f2148a72edd9cdd0e2
  • Loading branch information
rozele authored and facebook-github-bot committed Aug 18, 2017
1 parent 04c2f55 commit 809f1e9
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion Libraries/AppState/AppState.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,16 @@ class AppState extends NativeEventEmitter {
// the Android implementation.
this.currentState = RCTAppState.initialAppState || 'active';

let eventUpdated = false;

// TODO: this is a terrible solution - in order to ensure `currentState` prop
// is up to date, we have to register an observer that updates it whenever
// the state changes, even if nobody cares. We should just deprecate the
// `currentState` property and get rid of this.
this.addListener(
'appStateDidChange',
(appStateData) => {
eventUpdated = true;
this.currentState = appStateData.app_state;
}
);
Expand All @@ -118,7 +121,9 @@ class AppState extends NativeEventEmitter {
// and expose `getCurrentAppState` method directly.
RCTAppState.getCurrentAppState(
(appStateData) => {
this.currentState = appStateData.app_state;
if (!eventUpdated) {
this.currentState = appStateData.app_state;
}
},
logError
);
Expand Down

0 comments on commit 809f1e9

Please sign in to comment.