From 8d1eb62f0bf3c5014a632acbbf98f06d07cf666e Mon Sep 17 00:00:00 2001 From: ByteHamster Date: Sat, 15 Jul 2023 16:27:12 +0200 Subject: [PATCH] Delete partially downloaded file when giving up to retry (#6530) --- .../service/download/HttpDownloaderTest.java | 20 +++---- .../download/DownloadRequestCreator.java | 2 - .../download/EpisodeDownloadWorker.java | 18 +++++-- .../core/service/download/HttpDownloader.java | 26 --------- .../serviceinterface/DownloadRequest.java | 54 +++---------------- .../serviceinterface/DownloadRequestTest.java | 3 -- 6 files changed, 34 insertions(+), 89 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/service/download/HttpDownloaderTest.java b/app/src/androidTest/java/de/test/antennapod/service/download/HttpDownloaderTest.java index 76cba4706..aa55a1e59 100644 --- a/app/src/androidTest/java/de/test/antennapod/service/download/HttpDownloaderTest.java +++ b/app/src/androidTest/java/de/test/antennapod/service/download/HttpDownloaderTest.java @@ -72,12 +72,14 @@ public class HttpDownloaderTest { } private Downloader download(String url, String title, boolean expectedResult) { - return download(url, title, expectedResult, true, null, null, true); + return download(url, title, expectedResult, true, null, null); } - private Downloader download(String url, String title, boolean expectedResult, boolean deleteExisting, String username, String password, boolean deleteOnFail) { + private Downloader download(String url, String title, boolean expectedResult, boolean deleteExisting, + String username, String password) { FeedFile feedFile = setupFeedFile(url, title, deleteExisting); - DownloadRequest request = new DownloadRequest(feedFile.getFile_url(), url, title, 0, feedFile.getTypeAsInt(), username, password, deleteOnFail, null, false); + DownloadRequest request = new DownloadRequest(feedFile.getFile_url(), url, title, 0, feedFile.getTypeAsInt(), + username, password, null, false); Downloader downloader = new HttpDownloader(request); downloader.call(); DownloadResult status = downloader.getResult(); @@ -112,7 +114,8 @@ public class HttpDownloaderTest { public void testCancel() { final String url = httpServer.getBaseUrl() + "/delay/3"; FeedFileImpl feedFile = setupFeedFile(url, "delay", true); - final Downloader downloader = new HttpDownloader(new DownloadRequest(feedFile.getFile_url(), url, "delay", 0, feedFile.getTypeAsInt(), null, null, true, null, false)); + final Downloader downloader = new HttpDownloader(new DownloadRequest(feedFile.getFile_url(), url, "delay", 0, + feedFile.getTypeAsInt(), null, null, null, false)); Thread t = new Thread() { @Override public void run() { @@ -128,12 +131,11 @@ public class HttpDownloaderTest { } DownloadResult result = downloader.getResult(); assertFalse(result.isSuccessful()); - assertFalse(new File(feedFile.getFile_url()).exists()); } @Test public void testDeleteOnFailShouldDelete() { - Downloader downloader = download(url404, "testDeleteOnFailShouldDelete", false, true, null, null, true); + Downloader downloader = download(url404, "testDeleteOnFailShouldDelete", false, true, null, null); assertFalse(new File(downloader.getDownloadRequest().getDestination()).exists()); } @@ -143,18 +145,18 @@ public class HttpDownloaderTest { File dest = new File(destDir, filename); dest.delete(); assertTrue(dest.createNewFile()); - Downloader downloader = download(url404, filename, false, false, null, null, false); + Downloader downloader = download(url404, filename, false, false, null, null); assertTrue(new File(downloader.getDownloadRequest().getDestination()).exists()); } @Test public void testAuthenticationShouldSucceed() throws InterruptedException { - download(urlAuth, "testAuthSuccess", true, true, "user", "passwd", true); + download(urlAuth, "testAuthSuccess", true, true, "user", "passwd"); } @Test public void testAuthenticationShouldFail() { - Downloader downloader = download(urlAuth, "testAuthSuccess", false, true, "user", "Wrong passwd", true); + Downloader downloader = download(urlAuth, "testAuthSuccess", false, true, "user", "Wrong passwd"); assertEquals(DownloadError.ERROR_UNAUTHORIZED, downloader.getResult().getReason()); } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequestCreator.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequestCreator.java index 5ca904ff6..4d5f2883d 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequestCreator.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequestCreator.java @@ -28,7 +28,6 @@ public class DownloadRequestCreator { return new DownloadRequest.Builder(dest.toString(), feed) .withAuthentication(username, password) - .deleteOnFailure(true) .lastModified(feed.getLastUpdate()); } @@ -53,7 +52,6 @@ public class DownloadRequestCreator { ? media.getItem().getFeed().getPreferences().getPassword() : null; return new DownloadRequest.Builder(dest.toString(), media) - .deleteOnFailure(false) .withAuthentication(username, password); } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/EpisodeDownloadWorker.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/EpisodeDownloadWorker.java index c428bc861..62956894c 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/EpisodeDownloadWorker.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/EpisodeDownloadWorker.java @@ -134,6 +134,7 @@ public class EpisodeDownloadWorker extends Worker { } else { sendErrorNotification(); } + FileUtils.deleteQuietly(new File(downloader.getDownloadRequest().getDestination())); return Result.failure(); } @@ -162,6 +163,9 @@ public class EpisodeDownloadWorker extends Worker { Log.d(TAG, "Requested invalid range, restarting download from the beginning"); FileUtils.deleteQuietly(new File(downloader.getDownloadRequest().getDestination())); sendMessage(request.getTitle(), true); + if (isLastRunAttempt()) { + FileUtils.deleteQuietly(new File(downloader.getDownloadRequest().getDestination())); + } return retry3times(); } @@ -177,21 +181,29 @@ public class EpisodeDownloadWorker extends Worker { } else { sendErrorNotification(); } + FileUtils.deleteQuietly(new File(downloader.getDownloadRequest().getDestination())); return Result.failure(); } sendMessage(request.getTitle(), true); + if (isLastRunAttempt()) { + FileUtils.deleteQuietly(new File(downloader.getDownloadRequest().getDestination())); + } return retry3times(); } private Result retry3times() { - if (getRunAttemptCount() < 2) { - return Result.retry(); - } else { + if (isLastRunAttempt()) { sendErrorNotification(); return Result.failure(); + } else { + return Result.retry(); } } + private boolean isLastRunAttempt() { + return getRunAttemptCount() >= 2; + } + private void sendMessage(String episodeTitle, boolean retrying) { if (episodeTitle.length() > 20) { episodeTitle = episodeTitle.substring(0, 19) + "…"; diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/HttpDownloader.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/HttpDownloader.java index 6de5e7006..94cd81337 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/HttpDownloader.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/HttpDownloader.java @@ -49,12 +49,6 @@ public class HttpDownloader extends Downloader { File destination = new File(request.getDestination()); final boolean fileExists = destination.exists(); - if (request.isDeleteOnFailure() && fileExists) { - Log.w(TAG, "File already exists"); - onSuccess(); - return; - } - RandomAccessFile out = null; InputStream connection; ResponseBody responseBody = null; @@ -309,31 +303,11 @@ public class HttpDownloader extends Downloader { private void onFail(DownloadError reason, String reasonDetailed) { Log.d(TAG, "onFail() called with: " + "reason = [" + reason + "], reasonDetailed = [" + reasonDetailed + "]"); result.setFailed(reason, reasonDetailed); - if (request.isDeleteOnFailure()) { - cleanup(); - } } private void onCancelled() { Log.d(TAG, "Download was cancelled"); result.setCancelled(); cancelled = true; - cleanup(); - } - - /** - * Deletes unfinished downloads. - */ - private void cleanup() { - if (request.getDestination() != null) { - File dest = new File(request.getDestination()); - if (dest.exists()) { - boolean rc = dest.delete(); - Log.d(TAG, "Deleted file " + dest.getName() + "; Result: " - + rc); - } else { - Log.d(TAG, "cleanup() didn't delete file: does not exist."); - } - } } } diff --git a/net/download/service-interface/src/main/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequest.java b/net/download/service-interface/src/main/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequest.java index 9f9737edc..962ecfc84 100644 --- a/net/download/service-interface/src/main/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequest.java +++ b/net/download/service-interface/src/main/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequest.java @@ -14,7 +14,6 @@ import de.danoeh.antennapod.model.feed.FeedMedia; public class DownloadRequest implements Parcelable { public static final String REQUEST_ARG_PAGE_NR = "page"; - public static final String REQUEST_ARG_LOAD_ALL_PAGES = "loadAllPages"; private final String destination; private final String source; @@ -22,7 +21,6 @@ public class DownloadRequest implements Parcelable { private String username; private String password; private String lastModified; - private final boolean deleteOnFailure; private final long feedfileId; private final int feedfileType; private final Bundle arguments; @@ -35,26 +33,26 @@ public class DownloadRequest implements Parcelable { private boolean initiatedByUser; public DownloadRequest(@NonNull String destination, @NonNull String source, @NonNull String title, long feedfileId, - int feedfileType, String username, String password, boolean deleteOnFailure, + int feedfileType, String username, String password, Bundle arguments, boolean initiatedByUser) { - this(destination, source, title, feedfileId, feedfileType, null, deleteOnFailure, username, password, false, - arguments, initiatedByUser); + this(destination, source, title, feedfileId, feedfileType, null, username, password, false, + arguments, initiatedByUser); } private DownloadRequest(Builder builder) { this(builder.destination, builder.source, builder.title, builder.feedfileId, builder.feedfileType, - builder.lastModified, builder.deleteOnFailure, builder.username, builder.password, false, - builder.arguments != null ? builder.arguments : new Bundle(), builder.initiatedByUser); + builder.lastModified, builder.username, builder.password, false, + builder.arguments, builder.initiatedByUser); } private DownloadRequest(Parcel in) { this(in.readString(), in.readString(), in.readString(), in.readLong(), in.readInt(), in.readString(), - in.readByte() > 0, nullIfEmpty(in.readString()), nullIfEmpty(in.readString()), in.readByte() > 0, - in.readBundle(), in.readByte() > 0); + nullIfEmpty(in.readString()), nullIfEmpty(in.readString()), in.readByte() > 0, + in.readBundle(), in.readByte() > 0); } private DownloadRequest(String destination, String source, String title, long feedfileId, int feedfileType, - String lastModified, boolean deleteOnFailure, String username, String password, + String lastModified, String username, String password, boolean mediaEnqueued, Bundle arguments, boolean initiatedByUser) { this.destination = destination; this.source = source; @@ -62,7 +60,6 @@ public class DownloadRequest implements Parcelable { this.feedfileId = feedfileId; this.feedfileType = feedfileType; this.lastModified = lastModified; - this.deleteOnFailure = deleteOnFailure; this.username = username; this.password = password; this.mediaEnqueued = mediaEnqueued; @@ -83,7 +80,6 @@ public class DownloadRequest implements Parcelable { dest.writeLong(feedfileId); dest.writeInt(feedfileType); dest.writeString(lastModified); - dest.writeByte((deleteOnFailure) ? (byte) 1 : 0); // in case of null username/password, still write an empty string // (rather than skipping it). Otherwise, unmarshalling a collection // of them from a Parcel (from an Intent extra to submit a request to DownloadService) will fail. @@ -124,7 +120,6 @@ public class DownloadRequest implements Parcelable { if (lastModified != null ? !lastModified.equals(that.lastModified) : that.lastModified != null) return false; - if (deleteOnFailure != that.deleteOnFailure) return false; if (feedfileId != that.feedfileId) return false; if (feedfileType != that.feedfileType) return false; if (progressPercent != that.progressPercent) return false; @@ -151,7 +146,6 @@ public class DownloadRequest implements Parcelable { result = 31 * result + (username != null ? username.hashCode() : 0); result = 31 * result + (password != null ? password.hashCode() : 0); result = 31 * result + (lastModified != null ? lastModified.hashCode() : 0); - result = 31 * result + (deleteOnFailure ? 1 : 0); result = 31 * result + (int) (feedfileId ^ (feedfileId >>> 32)); result = 31 * result + feedfileType; result = 31 * result + arguments.hashCode(); @@ -237,26 +231,6 @@ public class DownloadRequest implements Parcelable { return lastModified; } - public boolean isDeleteOnFailure() { - return deleteOnFailure; - } - - public boolean isMediaEnqueued() { - return mediaEnqueued; - } - - public boolean isInitiatedByUser() { - return initiatedByUser; - } - - /** - * Set to true if the media is enqueued because of this download. - * The state is helpful if the download is cancelled, and undoing the enqueue is needed. - */ - public void setMediaEnqueued(boolean mediaEnqueued) { - this.mediaEnqueued = mediaEnqueued; - } - public Bundle getArguments() { return arguments; } @@ -268,7 +242,6 @@ public class DownloadRequest implements Parcelable { private String username; private String password; private String lastModified; - private boolean deleteOnFailure = false; private final long feedfileId; private final int feedfileType; private final Bundle arguments = new Bundle(); @@ -306,11 +279,6 @@ public class DownloadRequest implements Parcelable { } } - public Builder deleteOnFailure(boolean deleteOnFailure) { - this.deleteOnFailure = deleteOnFailure; - return this; - } - public Builder lastModified(String lastModified) { this.lastModified = lastModified; return this; @@ -322,12 +290,6 @@ public class DownloadRequest implements Parcelable { return this; } - public void loadAllPages(boolean loadAllPages) { - if (loadAllPages) { - arguments.putBoolean(REQUEST_ARG_LOAD_ALL_PAGES, true); - } - } - public DownloadRequest build() { return new DownloadRequest(this); } diff --git a/net/download/service-interface/src/test/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequestTest.java b/net/download/service-interface/src/test/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequestTest.java index 2709744f7..a48934cfe 100644 --- a/net/download/service-interface/src/test/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequestTest.java +++ b/net/download/service-interface/src/test/java/de/danoeh/antennapod/net/download/serviceinterface/DownloadRequestTest.java @@ -41,17 +41,14 @@ public class DownloadRequestTest { String password = "testPassword"; FeedMedia item = createFeedItem(1); DownloadRequest request1 = new DownloadRequest.Builder(destStr, item) - .deleteOnFailure(true) .withAuthentication(username, password) .build(); DownloadRequest request2 = new DownloadRequest.Builder(destStr, item) - .deleteOnFailure(true) .withAuthentication(username, password) .build(); DownloadRequest request3 = new DownloadRequest.Builder(destStr, item) - .deleteOnFailure(true) .withAuthentication("diffUsername", "diffPassword") .build();