MonitoringInstrumentation leaking activity instances through ExecutorService

[GoogleCodeExporter]

Library: com.android.support.test:testing-support-lib
Version: 0.1

TL;DR: on Android < 5.0, activity instances may be leaked for at most 60 
seconds by MonitoringInstrumentation after an activity started with 
MonitoringInstrumentation#startActivitySync is destroyed.

The fix is a one line change in MonitoringInstrumentation#onCreate().

I found this through a heap dump, here's what's going on:

* MonitoringInstrumentation#onCreate creates a thread pool executor using 
Executors.newCachedThreadPool();
* MonitoringInstrumentation.startActivitySync() posts a Future<Activity>. That 
ends up creating a FutureTask that has its "outcome" field set to an activity.

In an ideal world, once the FutureTask is completed it gets garbage collected 
and will therefore release the reference it has to the activity through the 
"outcome" field".

However, *prior to Android 5*, threads looping and blocking on queues may keep 
a reference to stack local variables until the queue unblock. Here's what I 
mean:

while(true) {
  Foo foo = blockingQueue.next();
  foo.doSomething();
}

Say blockingQueue only contains one thing and then stays empty. The loop will 
execute once and then block on next(). However, the first foo instance will 
never be garbage collected, because it's help as a stack local variable until 
the queue unblocks. It's a VM optimization that creates temporary memory leaks. 
This impacts any thread + blocking mechanism (thread pool executors, 
HandlerThread, etc) on Android versions less than 5 (can't repro on 5).

This is the same bug as this: https://github.com/square/picasso/pull/932

Because MonitoringInstrumentation uses Executors.newCachedThreadPool() so 
that's a core pool of 0 and a keep alive of 60 seconds. This means any thread 
that becomes idle will stay around for 60 seconds before being garbage 
collected.

So if we call MonitoringInstrumentation.startActivitySync(), the thread cool 
creates a thread to handle the FutureTask that will create the activity. Once 
that future task is done, the thread becomes idle. However, prior to Android 
5.0, it keeps a stack local reference to the future task, and therefore 
prevents the activity from being garbage collected (until that thread is reused 
for another future task, or GCed after 60 seconds).

The fix here is to configure the ThreadPoolExecutor so that threads are 
immediately killed when they become idle. It's not great, but it should be ok 
for instrumentation tests.

ie you could create it this way:

new ThreadPoolExecutor(0, Integer.MAX_VALUE, 0L, TimeUnit.SECONDS, new 
SynchronousQueue<Runnable>());

(same as Executors#newCachedThreadPool() except I replaced 60 with 0).

My current fix is to override Instrumentation#onCreate() and change the 
keepalive time on mExecutorService:

  @Override public void onCreate(Bundle arguments) {
    super.onCreate(arguments);
    try {
      Field mExecutorServiceField =
          MonitoringInstrumentation.class.getDeclaredField("mExecutorService");
      mExecutorServiceField.setAccessible(true);
      ThreadPoolExecutor executorService = (ThreadPoolExecutor) mExecutorServiceField.get(this);
      executorService.setKeepAliveTime(0, TimeUnit.MILLISECONDS);
    } catch (NoSuchFieldException | IllegalAccessException e) {
      throw new RuntimeException(e);
    }
  }


I initially filed this in the wrong place: 
https://code.google.com/p/android/issues/detail?id=161527

Original issue reported on code.google.com by p...@squareup.com on 18 May 2015 at 11:21

Attachments:

• heapdump_leak.png

[GoogleCodeExporter]

Original comment by nkors...@google.com on 22 May 2015 at 6:06

• Changed state: Accepted