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

RCTRefreshControl.m contains race condition #21762

Closed
rostislav-simonik opened this issue Oct 12, 2018 · 4 comments
Closed

RCTRefreshControl.m contains race condition #21762

rostislav-simonik opened this issue Oct 12, 2018 · 4 comments
Labels
Bug Resolution: Locked This issue was locked by the bot.

Comments

@rostislav-simonik
Copy link
Contributor

Environment

React Native Environment Info:
System:
OS: macOS 10.14
CPU: x64 Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
Memory: 56.93 MB / 16.00 GB
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 10.12.0 - /usr/local/bin/node
Yarn: 1.10.1 - /usr/local/bin/yarn
npm: 6.4.1 - /usr/local/bin/npm
Watchman: 4.9.0 - /usr/local/bin/watchman
SDKs:
iOS SDK:
Platforms: iOS 12.0, macOS 10.14, tvOS 12.0, watchOS 5.0
Android SDK:
Build Tools: 23.0.1, 23.0.3, 25.0.2, 25.0.3, 26.0.1, 26.0.2, 26.0.3
API Levels: 23, 25, 26
IDEs:
Android Studio: 3.0 AI-171.4443003
Xcode: 10.0/10A255 - /usr/bin/xcodebuild
npmPackages:
react: 16.6.0-alpha.8af6728 => 16.6.0-alpha.8af6728
react-native: 0.57.3 => 0.57.3
npmGlobalPackages:
create-react-native-app: 1.0.0
react-native-cli: 2.0.1
react-native-git-upgrade: 0.2.7

Description

Current implementation of RCTRefreshControl contains race condition which results in incorrect state of refresh control. The worst case scenario is that refresh control displays ongoing state and it's not possible to cancel or re-dispatch onRefresh event.

Logical scenario

  1. assign refreshing to true, which initiates beginRefreshingProgrammatically
  2. before beginRefreshingProgrammatically animation ends, change refreshing state by assigning refreshing to false.
    3 animation ends and overrides real state (refreshing: false) into previous state (refreshing: true)
@rostislav-simonik
Copy link
Contributor Author

Related to #5839

@alexhidalgo
Copy link

Similar issue
RN 0.55.4
iOS 11
iPhone 6 real device ONLY. Cannot reproduce on sim.

  1. Redux delivers state to refreshing prop on flatlist
  2. Redux state prints out correctly setting loading boolean to false
  3. spinner continues to display even if setState or forceUpdate (loading: false) is called manually in a subsequent timeout

Expected
Spinner hides once loading is set to false

Actual
Spinner continues to display even after redux sets loading to false on refreshing prop in flatlist

Setting a breakpoint resolves the issue. Sounds like a race condition due to my loading boolean being dependent on an async redux action.

@alexhidalgo
Copy link

Using requestAnimationFrame() to schedule my redux dispatch that eventually calls (loading: false) on refreshing seems to solve my issue. There is a screen transition immediately before I get to the view with the flatlist which made me think my specific issue is related to an animation race condition. It could also just be masking the issue by giving my redux action call a little time to set the refreshing prop on flatlist.

https://facebook.github.io/react-native/docs/timers

@stale
Copy link

stale bot commented Jan 20, 2019

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 stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 20, 2019
@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 24, 2019
@hramos hramos removed the Bug Report label Feb 6, 2019
grabbou pushed a commit that referenced this issue Mar 22, 2019
Summary:
Fixes #21762
Pull Request resolved: #21763

Differential Revision: D14064621

Pulled By: cpojer

fbshipit-source-id: 63010248a46cb49ed17ed89d7c55945aa7b22973
@facebook facebook locked as resolved and limited conversation to collaborators Feb 15, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Feb 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

4 participants