From 84adff9ae8cedae9afa0633495aceaf8a5c5a47b Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Mon, 20 Mar 2023 14:00:45 -0700 Subject: [PATCH] Pass an executor into ChromiumRequestSerializer ChromiumRequestSerializer creates an single thread executor for executing callbacks from cronet. In tests, we want to track all threads related to Glide and cronet through ActiveResource, so that the test result is deterministic. So we pass an active resource thread pool executor to ChromiumRequestSerializer, and the active resource will be tracked by ActiveResourceRule. PiperOrigin-RevId: 518069577 --- .../cronet/ChromiumRequestSerializer.java | 37 +++++++++++--- .../integration/cronet/ChromiumUrlLoader.java | 50 +++++++++++++++++-- .../cronet/ChromiumUrlFetcherTest.java | 4 +- 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumRequestSerializer.java b/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumRequestSerializer.java index 4daae3d608..afdbd83e19 100644 --- a/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumRequestSerializer.java +++ b/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumRequestSerializer.java @@ -91,14 +91,29 @@ public final int compareTo(PriorityRunnable another) { GLIDE_TO_CHROMIUM_PRIORITY.put(Priority.LOW, UrlRequest.Builder.REQUEST_PRIORITY_LOWEST); } - private final JobPool jobPool = new JobPool(); + private final JobPool jobPool; private final Map jobs = new HashMap<>(); private final CronetRequestFactory requestFactory; @Nullable private final DataLogger dataLogger; - ChromiumRequestSerializer(CronetRequestFactory requestFactory, @Nullable DataLogger dataLogger) { + ChromiumRequestSerializer( + CronetRequestFactory requestFactory, + @Nullable DataLogger dataLogger, + @Nullable final GlideExecutor executor) { this.requestFactory = requestFactory; this.dataLogger = dataLogger; + if (executor == null) { + this.jobPool = new JobPool(GLIDE_EXECUTOR_SUPPLIER); + } else { + this.jobPool = + new JobPool( + new Supplier() { + @Override + public Executor get() { + return executor; + } + }); + } } void startRequest(Priority priority, GlideUrl glideUrl, Listener listener) { @@ -178,6 +193,11 @@ private class Job extends Callback { private long responseStartTimeMs; private volatile boolean isCancelled; private BufferQueue.Builder builder; + private final Supplier executorSupplier; + + Job(Supplier executorSupplier) { + this.executorSupplier = executorSupplier; + } void init(GlideUrl glideUrl) { startTime = System.currentTimeMillis(); @@ -233,7 +253,7 @@ public void onReadCompleted( @Override public void onSucceeded(UrlRequest request, final UrlResponseInfo info) { - GLIDE_EXECUTOR_SUPPLIER + executorSupplier .get() .execute( new PriorityRunnable(priority) { @@ -251,7 +271,7 @@ public void run() { @Override public void onFailed( UrlRequest urlRequest, final UrlResponseInfo urlResponseInfo, final CronetException e) { - GLIDE_EXECUTOR_SUPPLIER + executorSupplier .get() .execute( new PriorityRunnable(priority) { @@ -264,7 +284,7 @@ public void run() { @Override public void onCanceled(UrlRequest urlRequest, @Nullable final UrlResponseInfo urlResponseInfo) { - GLIDE_EXECUTOR_SUPPLIER + executorSupplier .get() .execute( new PriorityRunnable(priority) { @@ -365,11 +385,16 @@ private void clearListeners() { private class JobPool { private static final int MAX_POOL_SIZE = 50; private final ArrayDeque pool = new ArrayDeque<>(); + private final Supplier executorSupplier; + + public JobPool(Supplier executorSupplier) { + this.executorSupplier = executorSupplier; + } public synchronized Job get(GlideUrl glideUrl) { Job job = pool.poll(); if (job == null) { - job = new Job(); + job = new Job(executorSupplier); } job.init(glideUrl); return job; diff --git a/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumUrlLoader.java b/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumUrlLoader.java index acae780028..8fac0f81c4 100644 --- a/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumUrlLoader.java +++ b/integration/cronet/src/main/java/com/bumptech/glide/integration/cronet/ChromiumUrlLoader.java @@ -3,6 +3,7 @@ import androidx.annotation.Nullable; import com.bumptech.glide.load.Options; import com.bumptech.glide.load.data.DataFetcher; +import com.bumptech.glide.load.engine.executor.GlideExecutor; import com.bumptech.glide.load.model.GlideUrl; import com.bumptech.glide.load.model.ModelLoader; import com.bumptech.glide.load.model.ModelLoaderFactory; @@ -12,7 +13,10 @@ import java.nio.ByteBuffer; /** - * An {@link com.bumptech.glide.load.model.ModelLoader} for loading urls using cronet. + * A {@link com.bumptech.glide.load.model.ModelLoader} for loading urls using cronet. + * + *

You can optionally pass an executor to the constructor for handling cronet callbacks in {@link + * ChromiumRequestSerializer}. If the executor is not provided, it will be created for you. * * @param The type of data this loader will load. */ @@ -29,7 +33,17 @@ public final class ChromiumUrlLoader implements ModelLoader { CronetRequestFactory requestFactory, @Nullable DataLogger dataLogger) { this.parser = parser; - requestSerializer = new ChromiumRequestSerializer(requestFactory, dataLogger); + requestSerializer = + new ChromiumRequestSerializer(requestFactory, dataLogger, /* executor= */ null); + } + + ChromiumUrlLoader( + ByteBufferParser parser, + CronetRequestFactory requestFactory, + @Nullable DataLogger dataLogger, + @Nullable GlideExecutor executor) { + this.parser = parser; + requestSerializer = new ChromiumRequestSerializer(requestFactory, dataLogger, executor); } @Override @@ -49,15 +63,29 @@ public static final class StreamFactory private CronetRequestFactory requestFactory; @Nullable private final DataLogger dataLogger; + @Nullable private final GlideExecutor executor; public StreamFactory(CronetRequestFactory requestFactory, @Nullable DataLogger dataLogger) { this.requestFactory = requestFactory; this.dataLogger = dataLogger; + this.executor = null; + } + + /** + * @param executor See {@link ChromiumUrlLoader} for details. + */ + public StreamFactory( + CronetRequestFactory requestFactory, + @Nullable DataLogger dataLogger, + @Nullable GlideExecutor executor) { + this.requestFactory = requestFactory; + this.dataLogger = dataLogger; + this.executor = executor; } @Override public ModelLoader build(MultiModelLoaderFactory multiFactory) { - return new ChromiumUrlLoader<>(this /*parser*/, requestFactory, dataLogger); + return new ChromiumUrlLoader<>(/* parser= */ this, requestFactory, dataLogger, executor); } @Override @@ -80,15 +108,29 @@ public static final class ByteBufferFactory private CronetRequestFactory requestFactory; @Nullable private final DataLogger dataLogger; + @Nullable private final GlideExecutor executor; public ByteBufferFactory(CronetRequestFactory requestFactory, @Nullable DataLogger dataLogger) { this.requestFactory = requestFactory; this.dataLogger = dataLogger; + this.executor = null; + } + + /** + * @param executor See {@link ChromiumUrlLoader} for details. + */ + public ByteBufferFactory( + CronetRequestFactory requestFactory, + @Nullable DataLogger dataLogger, + @Nullable GlideExecutor executor) { + this.requestFactory = requestFactory; + this.dataLogger = dataLogger; + this.executor = executor; } @Override public ModelLoader build(MultiModelLoaderFactory multiFactory) { - return new ChromiumUrlLoader<>(this /*parser*/, requestFactory, dataLogger); + return new ChromiumUrlLoader<>(/* parser= */ this, requestFactory, dataLogger, executor); } @Override diff --git a/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java b/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java index 8595e68e37..d4321062be 100644 --- a/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java +++ b/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java @@ -87,7 +87,9 @@ public ByteBuffer answer(InvocationOnMock invocation) throws Throwable { glideUrl = new GlideUrl("http://www.google.com"); urlRequestListenerCaptor = ArgumentCaptor.forClass(UrlRequest.Callback.class); - serializer = new ChromiumRequestSerializer(cronetRequestFactory, null /*dataLogger*/); + serializer = + new ChromiumRequestSerializer( + cronetRequestFactory, /* dataLogger= */ null, /* executor= */ null); fetcher = new ChromiumUrlFetcher<>(serializer, parser, glideUrl); builder = cronetEngine.newUrlRequestBuilder(