Skip to content

Commit

Permalink
Thread safety for ViewportManager list of ViewportChanged listeners
Browse files Browse the repository at this point in the history
Summary:
Adding and removing `ViewportChanged Listeners` into the `ViewportManager` can happen on any thread. For example, in `RecyclerCollectionComponentSpec`, we are adding a listener in `OnCreateInitialState`, which can happen on any thread.

This diff will fix this problem by making sure we synchronized on the operations around `List<ViewportChanged> mViewportChangedListeners`.

Reviewed By: indragiek

Differential Revision: D14390680

fbshipit-source-id: 74d5e060ee09a29ce7ff3ce854c1ca3f6a6ed480
  • Loading branch information
Desmond Ng authored and facebook-github-bot committed Mar 8, 2019
1 parent 33feef7 commit 9da9d90
Showing 1 changed file with 25 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.facebook.litho.widget;

import androidx.annotation.AnyThread;
import androidx.annotation.GuardedBy;
import androidx.annotation.Nullable;
import androidx.annotation.UiThread;
import androidx.recyclerview.widget.RecyclerView;
Expand Down Expand Up @@ -44,7 +46,8 @@ final class ViewportManager {
private int mTotalItemCount;
private boolean mShouldUpdate;

@Nullable private List<ViewportChanged> mViewportChangedListeners;
@GuardedBy("this")
private final List<ViewportChanged> mViewportChangedListeners;

private final LayoutInfo mLayoutInfo;
private final ViewportScrollListener mViewportScrollListener = new ViewportScrollListener();
Expand All @@ -57,6 +60,7 @@ final class ViewportManager {
mCurrentLastFullyVisiblePosition = layoutInfo.findLastFullyVisibleItemPosition();
mTotalItemCount = layoutInfo.getItemCount();
mLayoutInfo = layoutInfo;
mViewportChangedListeners = new ArrayList<>(2);
}

/**
Expand Down Expand Up @@ -91,12 +95,16 @@ void onViewportChanged(@ViewportInfo.State int state) {
mTotalItemCount = totalItemCount;
mShouldUpdate = false;

if (mViewportChangedListeners == null || mViewportChangedListeners.isEmpty()) {
return;
final List<ViewportChanged> viewportChangedListeners;
synchronized (this) {
if (mViewportChangedListeners.isEmpty()) {
return;
}
viewportChangedListeners = new ArrayList<>(mViewportChangedListeners);
}

for (int i = 0, size = mViewportChangedListeners.size(); i < size; i++) {
final ViewportChanged viewportChangedListener = mViewportChangedListeners.get(i);
for (int i = 0, size = viewportChangedListeners.size(); i < size; i++) {
final ViewportChanged viewportChangedListener = viewportChangedListeners.get(i);
viewportChangedListener.viewportChanged(
firstVisiblePosition,
lastVisiblePosition,
Expand Down Expand Up @@ -178,26 +186,30 @@ boolean shouldUpdate() {
return mCurrentFirstVisiblePosition < 0 || mCurrentLastVisiblePosition < 0 || mShouldUpdate;
}

@UiThread
@AnyThread
void addViewportChangedListener(@Nullable ViewportChanged viewportChangedListener) {
if (viewportChangedListener == null) {
return;
}

if (mViewportChangedListeners == null) {
mViewportChangedListeners = new ArrayList<>(2);
synchronized (this) {
mViewportChangedListeners.add(viewportChangedListener);
}

mViewportChangedListeners.add(viewportChangedListener);
}

@UiThread
@AnyThread
void removeViewportChangedListener(@Nullable ViewportChanged viewportChangedListener) {
if (viewportChangedListener == null || mViewportChangedListeners == null) {
if (viewportChangedListener == null) {
return;
}

mViewportChangedListeners.remove(viewportChangedListener);
synchronized (this) {
if (mViewportChangedListeners.isEmpty()) {
return;
}

mViewportChangedListeners.remove(viewportChangedListener);
}
}

@UiThread
Expand Down

0 comments on commit 9da9d90

Please sign in to comment.