Skip to content

Commit

Permalink
Add initial implementation of cleanup logic when a Sandbox is evicted
Browse files Browse the repository at this point in the history
Previously, when a Sandbox was evicted from SandboxManager, there was no logic
to shut down any of the associated resources. There was an implicit assumption
that the Sandbox would be garbage collected, and cleanup would magically occur,
but currently there are a lot of things preventing Sandbox GC from occurring,
and there are a lot of resources that need to be explicitly cleaned up.

Add a 'shutdown' method to Sandbox that closes the classloader and shuts down
the main executor thread. Although this is not currently sufficient to make the
Sandbox eligible for GC, it does free some resources.

Forthcoming CLs will add additional cleanup logic, with the eventual goal of
making Sandboxes eligible for GC when they are evicted from SandboxManager.

PiperOrigin-RevId: 564790888
  • Loading branch information
hoisie authored and Copybara-Service committed Sep 12, 2023
1 parent 2f276c1 commit c087df1
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 8 deletions.
Expand Up @@ -37,12 +37,17 @@ public SandboxManager(SandboxBuilder sandboxBuilder, SdkCollection sdkCollection
// We need to set the cache size of class loaders more than the number of supported APIs as
// different tests may have different configurations.
final int cacheSize = sdkCollection.getSupportedSdks().size() * CACHE_SIZE_FACTOR;
sandboxesByKey = new LinkedHashMap<SandboxKey, AndroidSandbox>() {
@Override
protected boolean removeEldestEntry(Map.Entry<SandboxKey, AndroidSandbox> eldest) {
return size() > cacheSize;
}
};
sandboxesByKey =
new LinkedHashMap<SandboxKey, AndroidSandbox>() {
@Override
protected boolean removeEldestEntry(Map.Entry<SandboxKey, AndroidSandbox> eldest) {
boolean toRemove = size() > cacheSize;
if (toRemove) {
eldest.getValue().shutdown();
}
return toRemove;
}
};
}

public synchronized AndroidSandbox getAndroidSandbox(
Expand Down
@@ -0,0 +1,56 @@
package org.robolectric;

import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.mock;

import java.util.concurrent.RejectedExecutionException;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runner.notification.RunNotifier;
import org.junit.runners.JUnit4;
import org.robolectric.internal.AndroidSandbox;

@RunWith(JUnit4.class)
public class RobolectricTestRunnerCleanupTest {

@Test
public void sandboxShutdownShouldCloseClassloader() throws Exception {
SingleSdkRobolectricTestRunner runner =
new SingleSdkRobolectricTestRunner(
TestWithEmptyTest.class, SingleSdkRobolectricTestRunner.defaultInjector().build());

runner.run(mock(RunNotifier.class));

AndroidSandbox latestSandbox = runner.getLatestSandbox();
latestSandbox.shutdown();
// Try to load a class that has not already been loaded. Closing a ClassLoader will only prevent
// loading new classes.
assertThrows(
ClassNotFoundException.class,
() ->
latestSandbox
.getRobolectricClassLoader()
.loadClass("com.android.server.am.ActivityManagerService"));
}

@Test
public void sandboxShutdownShouldShutDownMainThreadExecutor() throws Exception {
SingleSdkRobolectricTestRunner runner =
new SingleSdkRobolectricTestRunner(
TestWithEmptyTest.class, SingleSdkRobolectricTestRunner.defaultInjector().build());

runner.run(mock(RunNotifier.class));

AndroidSandbox latestSandbox = runner.getLatestSandbox();
latestSandbox.shutdown();
// Try to run something on the main thread executor, which has been shut down.
assertThrows(RejectedExecutionException.class, () -> latestSandbox.runOnMainThread(() -> {}));
}

@Ignore
public static class TestWithEmptyTest {
@Test
public void emptyTest() throws Exception {}
}
}
Expand Up @@ -4,7 +4,9 @@
import java.util.Collections;
import java.util.List;
import javax.annotation.Nonnull;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.InitializationError;
import org.robolectric.internal.AndroidSandbox;
import org.robolectric.pluginapi.Sdk;
import org.robolectric.pluginapi.SdkPicker;
import org.robolectric.pluginapi.UsesSdk;
Expand All @@ -16,6 +18,8 @@ public class SingleSdkRobolectricTestRunner extends RobolectricTestRunner {

private static final Injector DEFAULT_INJECTOR = defaultInjector().build();

private AndroidSandbox latestSandbox;

public static Injector.Builder defaultInjector() {
return RobolectricTestRunner.defaultInjector()
.bind(SdkPicker.class, SingleSdkPicker.class);
Expand All @@ -30,6 +34,17 @@ public SingleSdkRobolectricTestRunner(Class<?> testClass, Injector injector)
super(testClass, injector);
}

@Override
protected AndroidSandbox getSandbox(FrameworkMethod method) {
AndroidSandbox sandbox = super.getSandbox(method);
latestSandbox = sandbox;
return sandbox;
}

public AndroidSandbox getLatestSandbox() {
return latestSandbox;
}

@Override
ResModeStrategy getResModeStrategy() {
return ResModeStrategy.binary;
Expand Down
@@ -1,10 +1,11 @@
package org.robolectric.internal.bytecode;

import java.io.Closeable;
import java.io.InputStream;
import java.net.URL;

/** A provider of resources (à la ClassLoader). */
public interface ResourceProvider {
public interface ResourceProvider extends Closeable {

URL getResource(String resName);

Expand Down
@@ -1,8 +1,10 @@
package org.robolectric.internal.bytecode;

import static java.util.concurrent.TimeUnit.SECONDS;
import static org.robolectric.util.ReflectionHelpers.newInstance;
import static org.robolectric.util.ReflectionHelpers.setStaticField;

import java.io.IOException;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
Expand Down Expand Up @@ -101,6 +103,22 @@ public void runOnMainThread(Runnable runnable) {
});
}

/** Cleans up resources that have been opened by this Sandbox. */
public void shutdown() {
executorService.shutdown();

try {
executorService.awaitTermination(5, SECONDS);
} catch (InterruptedException e) {
throw new AssertionError(e);
}
try {
sandboxClassLoader.close();
} catch (IOException e) {
throw new AssertionError(e);
}
}

public <T> T runOnMainThread(Callable<T> callable) {
Future<T> future = executorService.submit(callable);
try {
Expand Down
Expand Up @@ -35,6 +35,7 @@ public class SandboxClassLoader extends URLClassLoader {
private final ClassInstrumentor classInstrumentor;
private final ClassNodeProvider classNodeProvider;
private final String dumpClassesDirectory;
private boolean isClosed;

/** Constructor for use by tests. */
SandboxClassLoader(InstrumentationConfiguration config) {
Expand Down Expand Up @@ -126,7 +127,9 @@ public Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundExce
if (loadedClass != null) {
return loadedClass;
}

if (isClosed) {
throw new ClassNotFoundException("This ClassLoader is closed");
}
if (config.shouldAcquire(name)) {
loadedClass =
PerfStatsCollector.getInstance()
Expand Down Expand Up @@ -205,4 +208,11 @@ private void ensurePackage(final String className) {
}
}
}

@Override
public void close() throws IOException {
super.close();
resourceProvider.close();
isClosed = true;
}
}

0 comments on commit c087df1

Please sign in to comment.