Skip to content

Commit

Permalink
Clear the error request in ErrorRequestCoordinator if it’s running.
Browse files Browse the repository at this point in the history
Previously we’d never clear the error request which could lead to a
StateVerifier exception or an object pooling error because 
primary.isLoadFailed() would always return false because the state would
be reset from failed to cleared in the call to primarly.clear() on the 
line immediately above the check.

Fixes #2767.
  • Loading branch information
sjudd committed Dec 28, 2017
1 parent ffd0fe0 commit 9c70aa5
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@
import com.bumptech.glide.load.engine.Resource;
import com.bumptech.glide.load.engine.executor.GlideExecutor;
import com.bumptech.glide.load.engine.executor.GlideExecutor.UncaughtThrowableStrategy;
import com.bumptech.glide.request.FutureTarget;
import com.bumptech.glide.request.RequestListener;
import com.bumptech.glide.test.ConcurrencyHelper;
import com.bumptech.glide.test.ResourceIds;
import com.bumptech.glide.test.TearDownGlide;
import com.bumptech.glide.test.WaitModelLoader;
import com.bumptech.glide.test.WaitModelLoader.WaitModel;
import java.io.File;
import java.util.concurrent.CountDownLatch;
import org.junit.Before;
Expand Down Expand Up @@ -106,6 +109,30 @@ public void load_whenLoadSucceeds_butEncoderFails_doesNotCallOnLoadFailed()
.onLoadFailed(any(GlideException.class), any(), anyDrawableTarget(), anyBoolean());
}

@Test
public void clearRequest_withError_afterPrimaryFails_clearsErrorRequest()
throws InterruptedException {
WaitModel<Integer> errorModel = WaitModelLoader.Factory.waitOn(ResourceIds.raw.canonical);

FutureTarget<Drawable> target =
Glide.with(context)
.load((Object) null)
.error(
Glide.with(context)
.load(errorModel)
.listener(requestListener))
.submit();

Glide.with(context).clear(target);
errorModel.countDown();

// Make sure any pending requests run.
concurrency.pokeMainThread();
Glide.tearDown();
// Make sure that any callbacks posted back to the main thread run.
concurrency.pokeMainThread();
}

private static final class WaitForErrorStrategy implements UncaughtThrowableStrategy {
final CountDownLatch latch = new CountDownLatch(1);
@Nullable Throwable error = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ public void pause() {
@Override
public void clear() {
primary.clear();
if (primary.isFailed()) {
// Don't check primary.isFailed() here because it will have been reset by the clear call
// immediately before this.
if (error.isRunning()) {
error.clear();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ public void clear_whenPrimaryHasNotFailed_doesNotClearError() {
}

@Test
public void clear_whenPrimaryHasFailed_clearsError() {
public void clear_whenPrimaryHasFailed_errorIsRunning_clearsError() {
when(primary.isFailed()).thenReturn(true);
when(error.isRunning()).thenReturn(true);
coordinator.clear();
verify(error).clear();
}
Expand All @@ -93,6 +94,14 @@ public void clear_whenPrimaryHasFailed_clearsPrimary() {
verify(primary).clear();
}

@Test
public void clear_whenErrorIsRunning_clearsError() {
when(error.isRunning()).thenReturn(true);
coordinator.clear();

verify(error).clear();
}

@Test
public void isPaused_primaryNotFailed_primaryNotPaused_returnsFalse() {
assertThat(coordinator.isPaused()).isFalse();
Expand Down

0 comments on commit 9c70aa5

Please sign in to comment.