Fix RefreshControl race condition

[janicduplessis]

There was a race condition with SwipeRefreshLayout that cause the RefreshControl to keep refreshing when it shouldn't.

It was caused because we have to use post to set the refreshing state otherwise it doesn't work when setting refreshing=true on initial mount. What happened is that post doesn't guarantee the order the runnables will be called so calling post with refreshing=true followed by refreshing=false caused the resfreshing=false runnable to be called before the resfreshing=true one. This made it stay in refreshing state when it should not.

D/test    ( 6171): setRefreshing true
W/ReactNativeJS( 6171): false
D/test    ( 6171): setRefreshing false
D/test    ( 6171): setRefreshing post false
D/test    ( 6171): setRefreshing post true

This change adds an instance variable and uses it in the post runnable to make sure the last set value is always used.

Test plan (required)
Tested that it fixed the issue in the original issue app and did some additional tests in UIExplorer example to make sure everything still worked properly.

Fixes #7313

[ghost]

By analyzing the blame information on this pull request, we identified @bestander and @andreicoman11 to be potential reviewers.

[brentvatne]

Seems reasonable to me, what do you think @bestander?

[andreicoman11]

Lgtm

[andreicoman11]

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[bestander]

@janicduplessis looks like this change breaks ReactSwipeRefreshLayoutTestCase instrumentation test that got open sourced just a couple of days ago.

[bestander]

I'll have to revert for now but please rebase and try again

[ghost]

@janicduplessis updated the pull request.

[janicduplessis]

Found out what the problem is, there is a dead lock with waitForBridgeAndUIIdle in the instrumentation test. I'll check if I can find out why it happens and fix it otherwise I got a workaround.

[bestander]

thanks, Janic!

On Fri, May 13, 2016 at 6:55 PM, Janic Duplessis [email protected]
wrote:

Found out what the problem is, there is a dead lock with
waitForBridgeAndUIIdle in the instrumentation test. I'll check if I can
find out why it happens and fix it otherwise I got a workaround.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7317 (comment)

[janicduplessis]

I think I got to the bottom of this, the test block when calling waitForIdleSync here. Seems like an issue with animation and waitForIdleSync, I assume the animation is the SwipeRefreshLayout spinner. There is a reported bug here. I couldn't really find a workaround it.

What was weird is that it worked before so I checked what my PR changed and found out that I added a check to only call post if the refreshing state changed. Removing that check makes the test work again.

Doesn't work

  @Override
  public void setRefreshing(boolean refreshing) {
    if (mRefreshing != refreshing) {
      mRefreshing = refreshing;
      // Use `post` otherwise the control won't start refreshing if refreshing is true when
      // the component gets mounted.
      post(new Runnable() {
        @Override
        public void run() {
          ReactSwipeRefreshLayout.super.setRefreshing(mRefreshing);
        }
      });
    }
  }

Works

  @Override
  public void setRefreshing(boolean refreshing) {
    mRefreshing = refreshing;
    // Use `post` otherwise the control won't start refreshing if refreshing is true when
    // the component gets mounted.
    post(new Runnable() {
      @Override
      public void run() {
        ReactSwipeRefreshLayout.super.setRefreshing(mRefreshing);
      }
    });
  }

To be honest I'm not really sure why it makes it work but eh :)

[ghost]

@janicduplessis updated the pull request.

[bestander]

Thanks for giving it a try, @janicduplessis.
The component/test is considered flaky by our internal tests right now, there IS something wrong.
I wonder if there is a race condition happening and your check just highlighted it.

The code change in this PR does not fix a race condition though.

[janicduplessis]

Yeah there is definetly something wrong, waitForIdleSync will block forever if it gets called while the SwipeRefreshLayout is animating. Changing the test to make it keep refreshing in the onRefresh callback will make the test fail 100% so I guess the race condition is between the SwipeRefreshLayout stopping refreshing and the first call to waitForIdleSync while it is animating.

One way of fixing it that I can see is using a simple sleep ~1sec before the assertion instead, it's a bit hacky but should work 100% since we can't rely on the normal wait technique that can block forever if a repeating animation is going on.

Do we want to merge this PR for now in address the test separatly? Right now it doesn't make it better or worse :)

[bestander]

To be fair, I would leave it as is until we get an improvement.
A ~1 sec wait may be a problem for CI tests? The emulator there is so slow, it takes 20 times slower to run some tests then on my local PC.
I honestly don't know enough about android animations and concurrency to make a call here, feel free to invoke other people to the discussion

[bestander]

Is this field useful?
It is read only once in the new Runnable below

[janicduplessis]

Yes, the problem is when setRefreshing gets called twice in the same bridge transaction the order the runnables get executed is bad, see the example in the PR desc, adding the field makes sure is uses the latest value.

[bestander]

Ah, I see thanks, that makes sense.

[bestander]

@janicduplessis, when you give it another look, could you create a new PR?
I am not sure what the shipit bot will do with the PR that was already improted.

[janicduplessis]

Sure, let's close this one.