Skip to content

Commit

Permalink
Add more robust cancellation support to MultiModelLoader.
Browse files Browse the repository at this point in the history
There's a race described in #3343 where a child DataFetcher can call the MultiModelLoader after it has been cancelled, which can result in us unnecessarily starting a new load. Although we should probably also change the fetcher to avoid calling its callback after it's cancelled, we can make this behavior less of a problem in general by changing MultiModelLoader instead of relying on every individual DataFetcher implementation.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=216546841
  • Loading branch information
sjudd committed Dec 17, 2018
1 parent c147f46 commit 3c9f92f
Showing 1 changed file with 14 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ static class MultiFetcher<Data> implements DataFetcher<Data>, DataCallback<Data>
@Nullable
private List<Throwable> exceptions;

private volatile boolean isCancelled;

MultiFetcher(
@NonNull List<DataFetcher<Data>> fetchers,
@NonNull Pool<List<Throwable>> throwableListPool) {
Expand All @@ -97,6 +99,14 @@ public void loadData(
this.callback = callback;
exceptions = throwableListPool.acquire();
fetchers.get(currentIndex).loadData(priority, this);

// If a race occurred where we cancelled the fetcher in cancel() and then called loadData here
// immediately after, make sure that we cancel the newly started fetcher. We don't bother
// checking cancelled before loadData because it's not required for correctness and would
// require an unlikely race to be useful.
if (isCancelled) {
cancel();
}
}

@Override
Expand All @@ -112,6 +122,7 @@ public void cleanup() {

@Override
public void cancel() {
isCancelled = true;
for (DataFetcher<Data> fetcher : fetchers) {
fetcher.cancel();
}
Expand Down Expand Up @@ -145,6 +156,9 @@ public void onLoadFailed(@NonNull Exception e) {
}

private void startNextOrFail() {
if (isCancelled) {
return;
}
if (currentIndex < fetchers.size() - 1) {
currentIndex++;
loadData(priority, callback);
Expand Down

0 comments on commit 3c9f92f

Please sign in to comment.