From 591b23b5ab465d8ce18bdeee823df74daba86d5e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 29 Mar 2016 19:19:09 +0200 Subject: [PATCH] Downloader.cancelDownload() instead of using external Thread logic This is needed so that downloads can be canceled from within an IntentService. Since the Downloader classes do not have any Thread logic in them, they shouldn't use Thread logic within them anyway. This also removes the unused argument to AsyncDownloader.attemptCancel(). --- .../java/org/fdroid/fdroid/AppDetails.java | 4 +- .../org/fdroid/fdroid/net/ApkDownloader.java | 6 +-- .../fdroid/net/AsyncDownloadWrapper.java | 6 +-- .../fdroid/fdroid/net/AsyncDownloader.java | 2 +- .../org/fdroid/fdroid/net/Downloader.java | 11 +++++- .../fdroid/fdroid/net/HttpDownloaderTest.java | 37 +++++++++++++++++++ 6 files changed, 55 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 9786e0437..7f2cd3e07 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -556,7 +556,7 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A @Override protected void onDestroy() { if (downloadHandler != null && !inProcessOfChangingConfiguration) { - downloadHandler.cancel(false); + downloadHandler.cancel(); cleanUpFinishedDownload(); } inProcessOfChangingConfiguration = false; @@ -1553,7 +1553,7 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A return; } - activity.downloadHandler.cancel(true); + activity.downloadHandler.cancel(); activity.cleanUpFinishedDownload(); setProgressVisible(false); updateViews(); diff --git a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java index 54b3bcc61..203c41893 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java @@ -253,12 +253,10 @@ public class ApkDownloader implements AsyncDownloader.Listener { /** * Attempts to cancel the download (if in progress) and also removes the progress * listener - * - * @param userRequested - true if the user requested the cancel (via button click), otherwise false. */ - public void cancel(boolean userRequested) { + public void cancel() { if (dlWrapper != null) { - dlWrapper.attemptCancel(userRequested); + dlWrapper.attemptCancel(); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java index 9d67c0b46..e56300c64 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java @@ -43,9 +43,9 @@ class AsyncDownloadWrapper extends Handler implements AsyncDownloader { downloadThread.start(); } - public void attemptCancel(boolean userRequested) { - if (downloadThread != null) { - downloadThread.interrupt(); + public void attemptCancel() { + if (downloader != null) { + downloader.cancelDownload(); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java index 0cf19d4b4..4f098c003 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java @@ -16,6 +16,6 @@ public interface AsyncDownloader { void download(); - void attemptCancel(boolean userRequested); + void attemptCancel(); } diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index 180d3aebd..f48b7e84a 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -23,6 +23,8 @@ public abstract class Downloader { public static final String EXTRA_BYTES_READ = "extraBytesRead"; public static final String EXTRA_TOTAL_BYTES = "extraTotalBytes"; + private volatile boolean cancelled = false; + private final OutputStream outputStream; private final File outputFile; @@ -124,12 +126,19 @@ public abstract class Downloader { * @throws InterruptedException */ private void throwExceptionIfInterrupted() throws InterruptedException { - if (Thread.interrupted()) { + if (cancelled) { Utils.debugLog(TAG, "Received interrupt, cancelling download"); throw new InterruptedException(); } } + /** + * Cancel a running download, triggering an {@link InterruptedException} + */ + public void cancelDownload() { + cancelled = true; + } + /** * This copies the downloaded data from the InputStream to the OutputStream, * keeping track of the number of bytes that have flowed through for the diff --git a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java index a12f34cd6..86284e5b2 100644 --- a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -6,8 +6,11 @@ import org.junit.Test; import java.io.File; import java.io.IOException; import java.net.URL; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class HttpDownloaderTest { @@ -56,4 +59,38 @@ public class HttpDownloaderTest { assertTrue(receivedProgress); } } + + @Test + public void downloadThenCancel() throws IOException, InterruptedException { + final CountDownLatch latch = new CountDownLatch(5); + URL url = new URL("https://f-droid.org/repo/index.jar"); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + final HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + httpDownloader.setListener(new Downloader.DownloaderProgressListener() { + @Override + public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + System.out.println("DownloaderProgressListener.sendProgress " + bytesRead + " / " + totalBytes); + receivedProgress = true; + latch.countDown(); + } + }); + new Thread(){ + @Override + public void run() { + try { + httpDownloader.download(); + fail(); + } catch (IOException e) { + e.printStackTrace(); + fail(); + } catch (InterruptedException e) { + // success! + } + } + }.start(); + latch.await(100, TimeUnit.SECONDS); // either 5 progress reports or 100 seconds + httpDownloader.cancelDownload(); + assertTrue(receivedProgress); + } }