Skip to content

Commit

Permalink
Poll ActiveResources’ ReferenceQueue using a separate Thread.
Browse files Browse the repository at this point in the history 
Avoids an issue where prolonged idle states on the main thread could 
lead to an unexpectedly large accumulation of cleared weak references
and a corresponding increase in memory usage.
  • Loading branch information
@sjudd
sjudd committed Nov 30, 2017
1 parent cb4bda0 commit 2c7aa85
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 64 deletions.
88 changes: 75 additions & 13 deletions library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java
View file
Original file line number Diff line number Diff line change
@@ -1,28 +1,52 @@
package com.bumptech.glide.load.engine;

import android.os.Handler;
import android.os.Handler.Callback;
import android.os.Looper;
import android.os.MessageQueue;
import android.os.Message;
import android.os.Process;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.VisibleForTesting;
import com.bumptech.glide.load.Key;
import com.bumptech.glide.load.engine.EngineResource.ResourceListener;
import com.bumptech.glide.util.Preconditions;
import com.bumptech.glide.util.Synthetic;
import com.bumptech.glide.util.Util;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.WeakReference;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.TimeUnit;

final class ActiveResources {
private static final int MSG_CLEAN_REF = 1;

private final boolean isActiveResourceRetentionAllowed;
private final Handler mainHandler = new Handler(Looper.getMainLooper(), new Callback() {
@Override
public boolean handleMessage(Message msg) {
if (msg.what == MSG_CLEAN_REF) {
cleanupActiveReference((ResourceWeakReference) msg.obj);
return true;
}
return false;
}
});
@VisibleForTesting
final Map<Key, ResourceWeakReference> activeEngineResources = new HashMap<>();

private ResourceListener listener;

// Lazily instantiate to avoid exceptions if Glide is initialized on a background thread. See
// #295.
@Nullable
private ReferenceQueue<EngineResource<?>> resourceReferenceQueue;
private ResourceListener listener;
@Nullable
private Thread cleanReferenceQueueThread;
private volatile boolean isShutdown;
@Nullable
private volatile DequeuedResourceCallback cb;

ActiveResources(boolean isActiveResourceRetentionAllowed) {
this.isActiveResourceRetentionAllowed = isActiveResourceRetentionAllowed;
Expand Down Expand Up @@ -68,6 +92,7 @@ EngineResource<?> get(Key key) {
}

private void cleanupActiveReference(@NonNull ResourceWeakReference ref) {
Util.assertMainThread();
activeEngineResources.remove(ref.key);

if (!ref.isCacheable || ref.resource == null) {
Expand All @@ -82,22 +107,59 @@ private void cleanupActiveReference(@NonNull ResourceWeakReference ref) {
private ReferenceQueue<EngineResource<?>> getReferenceQueue() {
if (resourceReferenceQueue == null) {
resourceReferenceQueue = new ReferenceQueue<>();
MessageQueue queue = Looper.myQueue();
queue.addIdleHandler(new RefQueueIdleHandler());
cleanReferenceQueueThread = new Thread(new Runnable() {
@SuppressWarnings("InfiniteLoopStatement")
@Override
public void run() {
Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);
ResourceWeakReference ref;
while (!isShutdown) {
try {
ref = (ResourceWeakReference) resourceReferenceQueue.remove();
mainHandler.obtainMessage(MSG_CLEAN_REF, ref).sendToTarget();

// This section for testing only.
DequeuedResourceCallback current = cb;
if (current != null) {
current.onResourceDequeued();
}
// End for testing only.
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
}
}, "glide-active-resources");
cleanReferenceQueueThread.start();
}
return resourceReferenceQueue;
}

// Responsible for cleaning up the active resource map by remove weak references that have been
// cleared.
private class RefQueueIdleHandler implements MessageQueue.IdleHandler {
@Override
public boolean queueIdle() {
ResourceWeakReference ref;
while ((ref = (ResourceWeakReference) getReferenceQueue().poll()) != null) {
cleanupActiveReference(ref);
@VisibleForTesting
void setEnqueuedResourceCallback(DequeuedResourceCallback cb) {
this.cb = cb;
}

@VisibleForTesting
interface DequeuedResourceCallback {
void onResourceDequeued();
}

@VisibleForTesting
void shutdown() {
isShutdown = true;
if (cleanReferenceQueueThread == null) {
return;
}

cleanReferenceQueueThread.interrupt();
try {
cleanReferenceQueueThread.join(TimeUnit.SECONDS.toMillis(5));
if (cleanReferenceQueueThread.isAlive()) {
throw new RuntimeException("Failed to join in time");
}
return true;
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}

Expand Down
1 change: 1 addition & 0 deletions library/src/main/java/com/bumptech/glide/load/engine/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ public void clearDiskCache() {
public void shutdown() {
engineJobFactory.shutdown();
diskCacheProvider.clearDiskCacheIfCreated();
activeResources.shutdown();
}

/**
Expand Down
75 changes: 49 additions & 26 deletions library/src/test/java/com/bumptech/glide/load/engine/ActiveResourcesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.verify;

import android.os.MessageQueue.IdleHandler;
import com.bumptech.glide.load.Key;
import com.bumptech.glide.load.engine.ActiveResources.DequeuedResourceCallback;
import com.bumptech.glide.load.engine.ActiveResources.ResourceWeakReference;
import com.bumptech.glide.load.engine.EngineResource.ResourceListener;
import com.bumptech.glide.tests.GlideShadowLooper;
import java.util.concurrent.CountDownLatch;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -20,6 +22,7 @@
import org.mockito.MockitoAnnotations;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;
import org.robolectric.shadows.ShadowLooper;

@RunWith(RobolectricTestRunner.class)
@Config(shadows = GlideShadowLooper.class)
Expand All @@ -40,6 +43,11 @@ public void setUp() {
reset(GlideShadowLooper.queue);
}

@After
public void tearDown() {
resources.shutdown();
}

@Test
public void get_withMissingKey_returnsNull() {
assertThat(resources.get(key)).isNull();
Expand Down Expand Up @@ -142,9 +150,7 @@ public void queueIdle_afterResourceRemovedFromActive_doesNotCallListener() {
ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
resources.deactivate(key);

weakRef.enqueue();

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}
Expand All @@ -156,9 +162,7 @@ public void queueIdle_withCacheableResourceInActive_callListener() {
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
weakRef.enqueue();

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

ArgumentCaptor<EngineResource<?>> captor = getEngineResourceCaptor();

Expand All @@ -180,22 +184,20 @@ public void queueIdle_withNotCacheableResourceInActive_doesNotCallListener() {

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
weakRef.enqueue();

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}

@Test
public void queueIdle_withCacheableResourceInActive_removesResourceFromActive() {
public void queueIdle_withCacheableResourceInActive_removesResourceFromActive()
throws InterruptedException {
EngineResource<Object> engineResource =
new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true);
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
weakRef.enqueue();

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

assertThat(resources.get(key)).isNull();
}
Expand All @@ -207,9 +209,7 @@ public void queueIdle_withNotCacheableResourceInActive_removesResourceFromActive
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
weakRef.enqueue();

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

assertThat(resources.get(key)).isNull();
}
Expand Down Expand Up @@ -245,11 +245,10 @@ public void queueIdle_withQueuedReferenceRetrievedFromGet_notifiesListener() {
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
weakRef.enqueue();

resources.get(key);

triggerQueueIdle();
enqueueAndWaitForRef(weakRef);

ArgumentCaptor<EngineResource<?>> captor = getEngineResourceCaptor();
verify(listener).onResourceReleased(eq(key), captor.capture());
Expand All @@ -263,11 +262,12 @@ public void queueIdle_withQueuedReferenceRetrievedFromGetAndNotCacheable_doesNot
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
CountDownLatch latch = getLatchForClearedRef();
weakRef.enqueue();

resources.get(key);

triggerQueueIdle();
waitForLatch(latch);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}
Expand All @@ -279,11 +279,12 @@ public void queueIdle_withQueuedReferenceDeactivated_doesNotNotifyListener() {
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
CountDownLatch latch = getLatchForClearedRef();
weakRef.enqueue();

resources.deactivate(key);

triggerQueueIdle();
waitForLatch(latch);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}
Expand All @@ -295,13 +296,14 @@ public void queueIdle_afterReferenceQueuedThenReactivated_doesNotNotifyListener(
resources.activate(key, first);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
CountDownLatch latch = getLatchForClearedRef();
weakRef.enqueue();

EngineResource<Object> second =
new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true);
resources.activate(key, second);

triggerQueueIdle();
waitForLatch(latch);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}
Expand Down Expand Up @@ -348,19 +350,40 @@ public void queueIdle_withQueuedReferenceRetrievedFromGet_retentionDisabled_does
resources.activate(key, engineResource);

ResourceWeakReference weakRef = resources.activeEngineResources.get(key);
CountDownLatch latch = getLatchForClearedRef();
weakRef.enqueue();

resources.get(key);

triggerQueueIdle();
waitForLatch(latch);

verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class));
}

private void triggerQueueIdle() {
ArgumentCaptor<IdleHandler> captor = ArgumentCaptor.forClass(IdleHandler.class);
verify(GlideShadowLooper.queue).addIdleHandler(captor.capture());
captor.getValue().queueIdle();
private void enqueueAndWaitForRef(ResourceWeakReference ref) {
CountDownLatch latch = getLatchForClearedRef();
ref.enqueue();
waitForLatch(latch);
}

private void waitForLatch(CountDownLatch latch) {
try {
latch.await();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
ShadowLooper.getShadowMainLooper().runToEndOfTasks();
}

private CountDownLatch getLatchForClearedRef() {
final CountDownLatch toWait = new CountDownLatch(1);
resources.setEnqueuedResourceCallback(new DequeuedResourceCallback() {
@Override
public void onResourceDequeued() {
toWait.countDown();
}
});
return toWait;
}

@SuppressWarnings("unchecked")
Expand Down
25 changes: 0 additions & 25 deletions library/src/test/java/com/bumptech/glide/load/engine/EngineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import android.os.MessageQueue.IdleHandler;
import com.bumptech.glide.GlideContext;
import com.bumptech.glide.Priority;
import com.bumptech.glide.load.DataSource;
Expand All @@ -39,7 +37,6 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import org.robolectric.RobolectricTestRunner;
Expand Down Expand Up @@ -471,28 +468,6 @@ public Object answer(InvocationOnMock invocationOnMock) throws Throwable {
verify(harness.cb).onResourceReady(any(Resource.class), eq(DataSource.MEMORY_CACHE));
}

@Test
public void load_afterResourceIsGcedFromActive_returnsFromMemoryCache() {
// clear previous calls to addIdleHandler
reset(GlideShadowLooper.queue);
when(harness.resource.getResource()).thenReturn(mock(Resource.class));
when(harness.resource.isCacheable()).thenReturn(true);
harness.cache = new LruResourceCache(100);
doAnswer(new Answer<Object>() {
@Override
public Object answer(InvocationOnMock invocationOnMock) throws Throwable {
harness.callOnEngineJobComplete();
return null;
}
}).when(harness.job).start(any(DecodeJob.class));
harness.doLoad();
ArgumentCaptor<IdleHandler> captor = ArgumentCaptor.forClass(IdleHandler.class);
verify(GlideShadowLooper.queue).addIdleHandler(captor.capture());
captor.getValue().queueIdle();
harness.doLoad();
verify(harness.cb).onResourceReady(any(Resource.class), eq(DataSource.MEMORY_CACHE));
}

@Test
public void load_withOnlyRetrieveFromCache_andPreviousNormalLoad_startsNewLoad() {
EngineJob<?> first = harness.job;
Expand Down

0 comments on commit 2c7aa85

Please sign in to comment.