Skip to content

Commit

Permalink
DownloadManager.Task: Remove top level thread interrupt
Browse files Browse the repository at this point in the history
DownloadManager doesn't know enough about the Downloader implementations to
know that interrupting the thread is the right thing to do. In particular,
if a Downloader implementation wants to delegate work to additional worker
threads, then it will probably not want its main thread to be interrupted
on cancelation. Instead, it will want to cancel/interrupt its worker threads,
and keep its main thread blocked until work on those worker threads has
stopped (download() must not return whilst the Downloader is still touching
the cache).

This change moves control over what happens to the individual Downloader
implementations.

Issue: #5978
PiperOrigin-RevId: 309419177
  • Loading branch information
ojw28 committed May 1, 2020
1 parent 32516d8 commit bb744e5
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1284,8 +1284,6 @@ public void cancel(boolean released) {
if (!isCanceled) {
isCanceled = true;
downloader.cancel();
// TODO - This will need propagating deeper as soon as we start using additional threads.
interrupt();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public final class ProgressiveDownloader implements Downloader {
private final CacheDataSource dataSource;
private final AtomicBoolean isCanceled;

@Nullable private volatile Thread downloadThread;

/**
* @param uri Uri of the data to be downloaded.
* @param customCacheKey A custom key that uniquely identifies the original stream. Used for cache
Expand Down Expand Up @@ -74,6 +76,10 @@ public ProgressiveDownloader(

@Override
public void download(@Nullable ProgressListener progressListener) throws IOException {
downloadThread = Thread.currentThread();
if (isCanceled.get()) {
return;
}
@Nullable PriorityTaskManager priorityTaskManager = dataSource.getUpstreamPriorityTaskManager();
if (priorityTaskManager != null) {
priorityTaskManager.add(C.PRIORITY_DOWNLOAD);
Expand All @@ -96,6 +102,10 @@ public void download(@Nullable ProgressListener progressListener) throws IOExcep
@Override
public void cancel() {
isCanceled.set(true);
@Nullable Thread downloadThread = this.downloadThread;
if (downloadThread != null) {
downloadThread.interrupt();
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ public int compareTo(Segment other) {
private final Executor executor;
private final AtomicBoolean isCanceled;

@Nullable private volatile Thread downloadThread;

/**
* @param manifestUri The {@link Uri} of the manifest to be downloaded.
* @param manifestParser A parser for the manifest.
Expand Down Expand Up @@ -103,6 +105,10 @@ public SegmentDownloader(

@Override
public final void download(@Nullable ProgressListener progressListener) throws IOException {
downloadThread = Thread.currentThread();
if (isCanceled.get()) {
return;
}
@Nullable
PriorityTaskManager priorityTaskManager =
cacheDataSourceFactory.getUpstreamPriorityTaskManager();
Expand Down Expand Up @@ -183,6 +189,10 @@ public final void download(@Nullable ProgressListener progressListener) throws I
@Override
public void cancel() {
isCanceled.set(true);
@Nullable Thread downloadThread = this.downloadThread;
if (downloadThread != null) {
downloadThread.interrupt();
}
}

@Override
Expand Down

0 comments on commit bb744e5

Please sign in to comment.