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

Add fix for refresh control state's race condition. #21763

Conversation

rostislav-simonik
Copy link
Contributor

@rostislav-simonik rostislav-simonik commented Oct 12, 2018

Fixes #21762

Test Plan:

  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)

Release Notes:

[BUGFIX][iOS][RefreshControl] Fix race condition that would overwrite state changes

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up 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 the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. 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 Oct 12, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@pull-bot
Copy link

pull-bot commented Oct 12, 2018

Messages
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS against 786c7fc

@matthargett
Copy link
Contributor

can you add an e2e test? also release notes like [BUGFIX][iOS][RefreshControl] Fix race condition that would overwrite state changes should suffice.

@rostislav-simonik
Copy link
Contributor Author

can you add an e2e test? also release notes like [BUGFIX][iOS][RefreshControl] Fix race condition that would overwrite state changes should suffice.

  • Release notes changed.
  • e2e test - Could you please give me an example, maybe another pull request. I've never done e2e test for this platform, but it's not problem to do it. Just need some directions.

@cpojer
Copy link
Contributor

cpojer commented Jan 22, 2019

Ping @matthargett: could you provide a quick guide for writing an e2e test for this PR?

Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see in the originating issues that this doesn’t repro in a simulator, only on a real device. As such, never mind the e2e test ask.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll land it. One thing to keep in mind here is because it is using local client time, changing the device time could break this behavior, no?

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 13, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Feb 13, 2019

Fails internally with:

error: unable to find developer root
react-native/React/Views/RCTRefreshControl.m:68:55: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(beginRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                      ^
react-native/React/Views/RCTRefreshControl.m:89:53: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(endRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                    ^
stderr: react-native/React/Views/RCTRefreshControl.m:68:55: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(beginRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                      ^
react-native/React/Views/RCTRefreshControl.m:89:53: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(endRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                    ^

Could you fix this up?

@matthargett do you know if there is a way to enable the same error on React Native GitHub? This seems to come up frequently.

@rostislav-simonik
Copy link
Contributor Author

Alright, I'll land it. One thing to keep in mind here is because it is using local client time, changing the device time could break this behavior, no?

Yes, I've got idea later that could be done by logical clock instead.

@rostislav-simonik
Copy link
Contributor Author

rostislav-simonik commented Feb 13, 2019

Fails internally with:

error: unable to find developer root
react-native/React/Views/RCTRefreshControl.m:68:55: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(beginRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                      ^
react-native/React/Views/RCTRefreshControl.m:89:53: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(endRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                    ^
stderr: react-native/React/Views/RCTRefreshControl.m:68:55: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(beginRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                      ^
react-native/React/Views/RCTRefreshControl.m:89:53: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
                       if(endRefreshingTimestamp == _currentRefreshingStateTimestamp) {
                                                    ^

Could you fix this up?

Done.

@cpojer
Copy link
Contributor

cpojer commented Feb 13, 2019

Thank you so much for the changes!

@matthargett could you do a second review of this given that it is now using a slightly different approach? Thank you.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 14, 2019
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

- (void)setCurrentRefreshingState:(BOOL)refreshing
{
_currentRefreshingState = refreshing;
_currentRefreshingStateTimestamp = _currentRefreshingStateClock++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentRefreshingStateClock is never decremented. Was this meant to track/prevent other potential races where it could go above 1 or 2?

Copy link
Contributor Author

@rostislav-simonik rostislav-simonik Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthargett no, it shouldn't because I used logical clock. Every time refreshSetCurrentRefreshingState is called i need unique timestamp. This time stamp is used in animation window which starts on
https://github.com/facebook/react-native/pull/21763/files#diff-cfafa8d7e42ab87ab863f917e187a4aeR56
and ends on
https://github.com/facebook/react-native/pull/21763/files#diff-cfafa8d7e42ab87ab863f917e187a4aeR70
and if timestamp is still the same, then state can be safely set, otherwise it should be thrown away due something else changed state and increased clock timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just changed that due i used local date time before, and as @cpojer has noted changing os date time can have impact on that logic. But in real life it wouldn't harm logic at all because chance that somebody changes os date time, and in that moment will trigger race condition described above, is astronomically low. But to prevent confusion, I've rid of that logic and replaced local date time with logical clock.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 15, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@rostislav-simonik merged commit 95d399b into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 15, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 15, 2019
@hramos hramos removed Import Failed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019
grabbou pushed a commit that referenced this pull request Mar 22, 2019
Summary:
Fixes #21762
Pull Request resolved: #21763

Differential Revision: D14064621

Pulled By: cpojer

fbshipit-source-id: 63010248a46cb49ed17ed89d7c55945aa7b22973
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants