Skip to content

Commit

Permalink
Pass an executor into ChromiumRequestSerializer
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sjudd authored and glide-copybara-robot committed Mar 20, 2023
1 parent 295d92d commit 84adff9
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<GlideUrl, Job> 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<Executor>() {
@Override
public Executor get() {
return executor;
}
});
}
}

void startRequest(Priority priority, GlideUrl glideUrl, Listener listener) {
Expand Down Expand Up @@ -178,6 +193,11 @@ private class Job extends Callback {
private long responseStartTimeMs;
private volatile boolean isCancelled;
private BufferQueue.Builder builder;
private final Supplier<Executor> executorSupplier;

Job(Supplier<Executor> executorSupplier) {
this.executorSupplier = executorSupplier;
}

void init(GlideUrl glideUrl) {
startTime = System.currentTimeMillis();
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -365,11 +385,16 @@ private void clearListeners() {
private class JobPool {
private static final int MAX_POOL_SIZE = 50;
private final ArrayDeque<Job> pool = new ArrayDeque<>();
private final Supplier<Executor> executorSupplier;

public JobPool(Supplier<Executor> 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*
* <p>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 <T> The type of data this loader will load.
*/
Expand All @@ -29,7 +33,17 @@ public final class ChromiumUrlLoader<T> implements ModelLoader<GlideUrl, T> {
CronetRequestFactory requestFactory,
@Nullable DataLogger dataLogger) {
this.parser = parser;
requestSerializer = new ChromiumRequestSerializer(requestFactory, dataLogger);
requestSerializer =
new ChromiumRequestSerializer(requestFactory, dataLogger, /* executor= */ null);
}

ChromiumUrlLoader(
ByteBufferParser<T> parser,
CronetRequestFactory requestFactory,
@Nullable DataLogger dataLogger,
@Nullable GlideExecutor executor) {
this.parser = parser;
requestSerializer = new ChromiumRequestSerializer(requestFactory, dataLogger, executor);
}

@Override
Expand All @@ -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<GlideUrl, InputStream> build(MultiModelLoaderFactory multiFactory) {
return new ChromiumUrlLoader<>(this /*parser*/, requestFactory, dataLogger);
return new ChromiumUrlLoader<>(/* parser= */ this, requestFactory, dataLogger, executor);
}

@Override
Expand All @@ -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<GlideUrl, ByteBuffer> build(MultiModelLoaderFactory multiFactory) {
return new ChromiumUrlLoader<>(this /*parser*/, requestFactory, dataLogger);
return new ChromiumUrlLoader<>(/* parser= */ this, requestFactory, dataLogger, executor);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 84adff9

Please sign in to comment.