From a43d5d8ef1d977c40900ef4a11cb70b9bf83d9f2 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Wed, 19 Jan 2022 15:07:46 -0300 Subject: [PATCH] Add glide support to download library --- app/build.gradle | 4 +- .../fdroid/fdroid/net/HttpDownloaderTest.java | 6 +- .../fdroid/fdroid/nearby/SwapSuccessView.java | 3 +- .../main/java/org/fdroid/fdroid/Utils.java | 2 +- .../main/java/org/fdroid/fdroid/data/App.java | 55 ++++++++++++++- .../java/org/fdroid/fdroid/data/Repo.java | 33 +++++++++ .../fdroid/fdroid/net/FDroidGlideModule.java | 20 ++++++ .../views/InstallConfirmActivity.java | 3 +- build.gradle | 1 + download/build.gradle | 4 ++ .../kotlin/org/fdroid/download/Downloader.kt | 1 + .../org/fdroid/download/HttpDownloader.kt | 12 +++- .../download/glide/DownloadRequestLoader.kt | 45 ++++++++++++ .../org/fdroid/download/glide/HttpFetcher.kt | 68 +++++++++++++++++++ .../download/glide/HttpGlideUrlLoader.kt | 44 ++++++++++++ .../org/fdroid/download/HttpDownloaderTest.kt | 27 ++++++++ .../kotlin/org/fdroid/download/HttpManager.kt | 62 ++++++++++++----- .../org/fdroid/download/NoResumeException.kt | 3 + .../org/fdroid/download/HttpManagerTest.kt | 4 +- 19 files changed, 364 insertions(+), 33 deletions(-) create mode 100644 download/src/androidMain/kotlin/org/fdroid/download/glide/DownloadRequestLoader.kt create mode 100644 download/src/androidMain/kotlin/org/fdroid/download/glide/HttpFetcher.kt create mode 100644 download/src/androidMain/kotlin/org/fdroid/download/glide/HttpGlideUrlLoader.kt create mode 100644 download/src/commonMain/kotlin/org/fdroid/download/NoResumeException.kt diff --git a/app/build.gradle b/app/build.gradle index cd2cdeb6a..774158d9f 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -177,8 +177,8 @@ dependencies { implementation 'com.fasterxml.jackson.core:jackson-annotations:2.11.1' implementation 'com.fasterxml.jackson.core:jackson-databind:2.11.1' - implementation 'com.github.bumptech.glide:glide:4.12.0' - annotationProcessor 'com.github.bumptech.glide:compiler:4.12.0' + implementation "com.github.bumptech.glide:glide:$glide_version" + annotationProcessor "com.github.bumptech.glide:compiler:$glide_version" implementation 'org.bouncycastle:bcprov-jdk15on:1.65' fullImplementation 'org.bouncycastle:bcpkix-jdk15on:1.65' diff --git a/app/src/androidTest/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/androidTest/java/org/fdroid/fdroid/net/HttpDownloaderTest.java index 429c77873..f4d9bb832 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/net/HttpDownloaderTest.java +++ b/app/src/androidTest/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -125,7 +125,8 @@ public class HttpDownloaderTest { String path = "myusername/supersecretpassword"; List mirrors = Mirror.fromStrings(Collections.singletonList("https://httpbin.org/basic-auth/")); File destFile = File.createTempFile("dl-", ""); - final DownloadRequest request = new DownloadRequest(path, mirrors, null,"myusername", "wrongpassword"); + final DownloadRequest request = + new DownloadRequest(path, mirrors, null, "myusername", "wrongpassword"); HttpDownloader httpDownloader = new HttpDownloader(httpManager, request, destFile); httpDownloader.download(); assertFalse(destFile.exists()); @@ -137,7 +138,8 @@ public class HttpDownloaderTest { String path = "myusername/supersecretpassword"; List mirrors = Mirror.fromStrings(Collections.singletonList("https://httpbin.org/basic-auth/")); File destFile = File.createTempFile("dl-", ""); - final DownloadRequest request = new DownloadRequest(path, mirrors, null, "wrongusername", "supersecretpassword"); + final DownloadRequest request = + new DownloadRequest(path, mirrors, null, "wrongusername", "supersecretpassword"); HttpDownloader httpDownloader = new HttpDownloader(httpManager, request, destFile); httpDownloader.download(); assertFalse(destFile.exists()); diff --git a/app/src/full/java/org/fdroid/fdroid/nearby/SwapSuccessView.java b/app/src/full/java/org/fdroid/fdroid/nearby/SwapSuccessView.java index c9647d173..fba7d790f 100644 --- a/app/src/full/java/org/fdroid/fdroid/nearby/SwapSuccessView.java +++ b/app/src/full/java/org/fdroid/fdroid/nearby/SwapSuccessView.java @@ -313,8 +313,7 @@ public class SwapSuccessView extends SwapView implements LoaderManager.LoaderCal nameView.setText(app.name); } - Glide.with(iconView.getContext()) - .load(app.getIconUrl(iconView.getContext())) + app.loadWithGlide(iconView.getContext()) .apply(Utils.getAlwaysShowIconRequestOptions()) .into(iconView); diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index 816be3fb2..dbe47a1e3 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -491,7 +491,7 @@ public final class Utils { .fallback(R.drawable.ic_repo_app_default); } iconRequestOptions.onlyRetrieveFromCache(!Preferences.get().isBackgroundDownloadAllowed()); - Glide.with(context).load(app.getIconUrl(iv.getContext())).apply(iconRequestOptions).into(iv); + app.loadWithGlide(context).apply(iconRequestOptions).into(iv); } /** diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index 10338a4d6..0697dd9ce 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -10,6 +10,7 @@ import android.content.res.AssetManager; import android.content.res.Resources; import android.content.res.XmlResourceParser; import android.database.Cursor; +import android.graphics.drawable.Drawable; import android.net.Uri; import android.os.Build; import android.os.Environment; @@ -19,11 +20,15 @@ import android.os.Parcelable; import android.text.TextUtils; import android.util.Log; +import com.bumptech.glide.Glide; +import com.bumptech.glide.RequestBuilder; import com.fasterxml.jackson.annotation.JacksonInject; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.commons.io.filefilter.RegexFileFilter; +import org.fdroid.download.DownloadRequest; +import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; @@ -733,8 +738,18 @@ public class App extends ValueObject implements Comparable, Parcelable { .build(); } - public String getIconUrl(Context context) { + public RequestBuilder loadWithGlide(Context context) { Repo repo = RepoProvider.Helper.findById(context, repoId); + if (repo.address.startsWith("content://")) { + return Glide.with(context).load(getIconUrl(context, repo)); + } else { + return Glide.with(context).load(getDownloadRequest(context, repo)); + } + } + + @Nullable + @Deprecated // not taking mirrors into account + public String getIconUrl(Context context, Repo repo) { if (TextUtils.isEmpty(iconUrl)) { if (TextUtils.isEmpty(iconFromApk)) { return null; @@ -755,6 +770,44 @@ public class App extends ValueObject implements Comparable, Parcelable { return repo.getFileUrl(packageName, iconUrl); } + @Nullable + @Deprecated // not taking mirrors into account + public String getIconUrl(Context context) { + Repo repo = RepoProvider.Helper.findById(context, repoId); + return getIconUrl(context, repo); + } + + @Nullable + public DownloadRequest getDownloadRequest(Context context, Repo repo) { + String path; + if (TextUtils.isEmpty(iconUrl)) { + if (TextUtils.isEmpty(iconFromApk)) { + return null; + } + if (iconFromApk.endsWith(".xml")) { + // We cannot use xml resources as icons. F-Droid server should not include them + // https://gitlab.com/fdroid/fdroidserver/issues/344 + return null; + } + String iconsDir; + if (repo.version >= Repo.VERSION_DENSITY_SPECIFIC_ICONS) { + iconsDir = Utils.getIconsDir(context, 1.0); + } else { + iconsDir = Utils.FALLBACK_ICONS_DIR; + } + path = repo.getPath(iconsDir, iconFromApk); + } else { + path = repo.getPath(packageName, iconUrl); + } + return repo.getDownloadRequest(path); + } + + @Nullable + public DownloadRequest getDownloadRequest(Context context) { + Repo repo = RepoProvider.Helper.findById(context, repoId); + return getDownloadRequest(context, repo); + } + public String getFeatureGraphicUrl(Context context) { if (TextUtils.isEmpty(featureGraphic)) { return null; diff --git a/app/src/main/java/org/fdroid/fdroid/data/Repo.java b/app/src/main/java/org/fdroid/fdroid/data/Repo.java index bcbb2f58c..90a0d6798 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Repo.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Repo.java @@ -30,18 +30,24 @@ import android.text.TextUtils; import com.fasterxml.jackson.annotation.JsonIgnore; +import org.fdroid.download.DownloadRequest; +import org.fdroid.download.Mirror; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Schema.RepoTable.Cols; import org.fdroid.fdroid.net.TreeUriDownloader; import java.net.MalformedURLException; +import java.net.Proxy; import java.net.URL; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.HashSet; import java.util.List; +import info.guardianproject.netcipher.NetCipher; + /** * Represents a the descriptive info and metadata about a given repo, as provided @@ -268,6 +274,27 @@ public class Repo extends ValueObject { return tempName; } + /** + * Gets the path relative to the repo root. + * Can be used to create URLs for use with mirrors. + * Attention: This does NOT encode for use in URLs. + */ + public String getPath(String... pathElements) { + /* Each String in pathElements might contain a /, should keep these as path elements */ + ArrayList elements = new ArrayList<>(); + for (String element : pathElements) { + Collections.addAll(elements, element.split("/")); + } + // build up path WITHOUT encoding the segments, this will happen later when turned into URL + StringBuilder sb = new StringBuilder(); + for (String element : elements) { + sb.append(element).append("/"); + } + sb.deleteCharAt(sb.length() - 1); // remove trailing slash + return sb.toString(); + } + + @Deprecated // not taking mirrors into account public String getFileUrl(String... pathElements) { /* Each String in pathElements might contain a /, should keep these as path elements */ List elements = new ArrayList(); @@ -302,6 +329,12 @@ public class Repo extends ValueObject { } } + public DownloadRequest getDownloadRequest(String path) { + List mirrors = Mirror.fromStrings(getMirrorList()); + Proxy proxy = NetCipher.getProxy(); + return new DownloadRequest(path, mirrors, proxy, username, password); + } + private static int toInt(Integer value) { if (value == null) { return 0; diff --git a/app/src/main/java/org/fdroid/fdroid/net/FDroidGlideModule.java b/app/src/main/java/org/fdroid/fdroid/net/FDroidGlideModule.java index 6989f1ba3..fbf6cc35a 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/FDroidGlideModule.java +++ b/app/src/main/java/org/fdroid/fdroid/net/FDroidGlideModule.java @@ -4,19 +4,29 @@ import android.content.Context; import android.graphics.Bitmap; import android.graphics.drawable.Drawable; +import com.bumptech.glide.Glide; import com.bumptech.glide.GlideBuilder; +import com.bumptech.glide.Registry; import com.bumptech.glide.annotation.GlideModule; import com.bumptech.glide.load.DecodeFormat; +import com.bumptech.glide.load.model.GlideUrl; import com.bumptech.glide.load.resource.bitmap.BitmapTransitionOptions; import com.bumptech.glide.load.resource.drawable.DrawableTransitionOptions; import com.bumptech.glide.module.AppGlideModule; import com.bumptech.glide.request.RequestOptions; +import org.fdroid.download.DownloadRequest; +import org.fdroid.download.glide.DownloadRequestLoader; +import org.fdroid.download.glide.HttpGlideUrlLoader; import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Preferences; import androidx.annotation.NonNull; +import java.io.InputStream; + +import info.guardianproject.netcipher.NetCipher; + /** * The one time initialization of Glide. */ @@ -32,4 +42,14 @@ public class FDroidGlideModule extends AppGlideModule { .onlyRetrieveFromCache(!Preferences.get().isBackgroundDownloadAllowed()) .timeout(FDroidApp.getTimeout())); } + + @Override + public void registerComponents(@NonNull Context context, @NonNull Glide glide, Registry registry) { + HttpGlideUrlLoader.Factory urlLoaderFactory = + new HttpGlideUrlLoader.Factory(DownloaderFactory.HTTP_MANAGER, NetCipher::getProxy); + registry.replace(GlideUrl.class, InputStream.class, urlLoaderFactory); + DownloadRequestLoader.Factory requestLoaderFactory = + new DownloadRequestLoader.Factory(DownloaderFactory.HTTP_MANAGER); + registry.append(DownloadRequest.class, InputStream.class, requestLoaderFactory); + } } diff --git a/app/src/main/java/org/fdroid/fdroid/privileged/views/InstallConfirmActivity.java b/app/src/main/java/org/fdroid/fdroid/privileged/views/InstallConfirmActivity.java index 88849bb43..0b656eb96 100644 --- a/app/src/main/java/org/fdroid/fdroid/privileged/views/InstallConfirmActivity.java +++ b/app/src/main/java/org/fdroid/fdroid/privileged/views/InstallConfirmActivity.java @@ -79,8 +79,7 @@ public class InstallConfirmActivity extends AppCompatActivity implements OnCance TabHost tabHost = (TabHost) findViewById(android.R.id.tabhost); appName.setText(app.name); - Glide.with(this) - .load(app.getIconUrl(this)) + app.loadWithGlide(this) .apply(Utils.getAlwaysShowIconRequestOptions()) .into(appIcon); diff --git a/build.gradle b/build.gradle index 196c2ed76..b4f3ab20e 100644 --- a/build.gradle +++ b/build.gradle @@ -1,6 +1,7 @@ buildscript { ext { kotlin_version = "1.6.10" + glide_version = "4.12.0" } repositories { mavenCentral() diff --git a/download/build.gradle b/download/build.gradle index b2d715350..a5dfc5ba5 100644 --- a/download/build.gradle +++ b/download/build.gradle @@ -56,6 +56,10 @@ kotlin { androidMain { dependencies { implementation "io.ktor:ktor-client-okhttp:$ktor_version" + implementation("com.github.bumptech.glide:glide:$glide_version") { + transitive = false // we don't need all that it pulls in, just the basics + } + implementation "com.github.bumptech.glide:annotations:$glide_version" implementation 'ch.qos.logback:logback-classic:1.2.5' } } diff --git a/download/src/androidMain/kotlin/org/fdroid/download/Downloader.kt b/download/src/androidMain/kotlin/org/fdroid/download/Downloader.kt index 313974711..9336eb779 100644 --- a/download/src/androidMain/kotlin/org/fdroid/download/Downloader.kt +++ b/download/src/androidMain/kotlin/org/fdroid/download/Downloader.kt @@ -110,6 +110,7 @@ abstract class Downloader constructor( } @Suppress("BlockingMethodInNonBlockingContext") + @Throws(InterruptedException::class, IOException::class, NoResumeException::class) protected suspend fun downloadFromBytesReceiver(isResume: Boolean) { try { FileOutputStream(outputFile, isResume).use { outputStream -> diff --git a/download/src/androidMain/kotlin/org/fdroid/download/HttpDownloader.kt b/download/src/androidMain/kotlin/org/fdroid/download/HttpDownloader.kt index 3c604b7ec..6b398884f 100644 --- a/download/src/androidMain/kotlin/org/fdroid/download/HttpDownloader.kt +++ b/download/src/androidMain/kotlin/org/fdroid/download/HttpDownloader.kt @@ -65,7 +65,7 @@ class HttpDownloader constructor( throw NotImplementedError("Use getInputStreamSuspend instead.") } - @Throws(IOException::class) + @Throws(IOException::class, NoResumeException::class) override suspend fun getBytes(resumable: Boolean, receiver: (ByteArray) -> Unit) { val skipBytes = if (resumable) outputFile.length() else null return try { @@ -161,7 +161,15 @@ class HttpDownloader constructor( resumable = true } log.debug { "downloading ${request.path} (is resumable: $resumable)" } - runBlocking { downloadFromBytesReceiver(resumable) } + runBlocking { + try { + downloadFromBytesReceiver(resumable) + } catch (e: NoResumeException) { + require(resumable) { "Got $e even though download was not resumable" } + if (!outputFile.delete()) log.warn { "Warning: " + outputFile.absolutePath + " not deleted" } + downloadFromBytesReceiver(false) + } + } } @TargetApi(24) diff --git a/download/src/androidMain/kotlin/org/fdroid/download/glide/DownloadRequestLoader.kt b/download/src/androidMain/kotlin/org/fdroid/download/glide/DownloadRequestLoader.kt new file mode 100644 index 000000000..bc9c82689 --- /dev/null +++ b/download/src/androidMain/kotlin/org/fdroid/download/glide/DownloadRequestLoader.kt @@ -0,0 +1,45 @@ +package org.fdroid.download.glide + +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.model.ModelLoader +import com.bumptech.glide.load.model.ModelLoader.LoadData +import com.bumptech.glide.load.model.ModelLoaderFactory +import com.bumptech.glide.load.model.MultiModelLoaderFactory +import com.bumptech.glide.signature.ObjectKey +import org.fdroid.download.DownloadRequest +import org.fdroid.download.HttpManager +import java.io.InputStream + +class DownloadRequestLoader( + private val httpManager: HttpManager, +) : ModelLoader { + + override fun handles(downloadRequest: DownloadRequest): Boolean { + return true + } + + override fun buildLoadData( + downloadRequest: DownloadRequest, + width: Int, + height: Int, + options: Options, + ): LoadData { + return LoadData(downloadRequest.getKey(), HttpFetcher(httpManager, downloadRequest)) + } + + class Factory( + private val httpManager: HttpManager, + ) : ModelLoaderFactory { + override fun build(multiFactory: MultiModelLoaderFactory): ModelLoader { + return DownloadRequestLoader(httpManager) + } + + override fun teardown() {} + } + +} + +fun DownloadRequest.getKey(): ObjectKey { + // TODO should we choose a unique key or is it ok for this to work cross-repo based on file path only? + return ObjectKey(path) +} diff --git a/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpFetcher.kt b/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpFetcher.kt new file mode 100644 index 000000000..22f74c43a --- /dev/null +++ b/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpFetcher.kt @@ -0,0 +1,68 @@ +package org.fdroid.download.glide + +import android.net.Uri +import com.bumptech.glide.Priority +import com.bumptech.glide.load.DataSource +import com.bumptech.glide.load.data.DataFetcher +import com.bumptech.glide.load.model.GlideUrl +import io.ktor.client.engine.ProxyConfig +import io.ktor.utils.io.jvm.javaio.toInputStream +import kotlinx.coroutines.DelicateCoroutinesApi +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch +import org.fdroid.download.DownloadRequest +import org.fdroid.download.HttpManager +import org.fdroid.download.Mirror +import java.io.InputStream + +class HttpFetcher( + private val httpManager: HttpManager, + private val downloadRequest: DownloadRequest, +) : DataFetcher { + + @Deprecated("Use DownloadRequests with other constructor instead") + constructor( + httpManager: HttpManager, + glideUrl: GlideUrl, proxy: ProxyConfig?, + ) : this(httpManager, getDownloadRequest(glideUrl, proxy)) + + companion object { + private fun getDownloadRequest(glideUrl: GlideUrl, proxy: ProxyConfig?): DownloadRequest { + val (mirror, path) = glideUrl.toStringUrl().split("/repo/") + return DownloadRequest(Uri.decode(path), listOf(Mirror("$mirror/repo")), proxy) + } + } + + var job: Job? = null + + @OptIn(DelicateCoroutinesApi::class) + override fun loadData(priority: Priority, callback: DataFetcher.DataCallback) { + job = GlobalScope.launch(Dispatchers.IO) { + try { + // glide should take care of closing this stream and the underlying channel + val inputStream = httpManager.getChannel(downloadRequest).toInputStream() + callback.onDataReady(inputStream) + } catch (e: Exception) { + callback.onLoadFailed(e) + } + } + } + + override fun cleanup() { + job = null + } + + override fun cancel() { + job?.cancel() + } + + override fun getDataClass(): Class { + return InputStream::class.java + } + + override fun getDataSource(): DataSource { + return DataSource.REMOTE + } +} diff --git a/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpGlideUrlLoader.kt b/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpGlideUrlLoader.kt new file mode 100644 index 000000000..64e03541d --- /dev/null +++ b/download/src/androidMain/kotlin/org/fdroid/download/glide/HttpGlideUrlLoader.kt @@ -0,0 +1,44 @@ +package org.fdroid.download.glide + +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.model.GlideUrl +import com.bumptech.glide.load.model.ModelLoader +import com.bumptech.glide.load.model.ModelLoader.LoadData +import com.bumptech.glide.load.model.ModelLoaderFactory +import com.bumptech.glide.load.model.MultiModelLoaderFactory +import io.ktor.client.engine.ProxyConfig +import mu.KotlinLogging +import org.fdroid.download.HttpManager +import java.io.InputStream + +@Deprecated("Use DownloadRequestLoader instead") +class HttpGlideUrlLoader( + private val httpManager: HttpManager, + private val proxyGetter: () -> ProxyConfig?, +) : ModelLoader { + + companion object { + private val log = KotlinLogging.logger { } + } + + override fun handles(url: GlideUrl): Boolean { + return true + } + + override fun buildLoadData(glideUrl: GlideUrl, width: Int, height: Int, options: Options): LoadData { + log.warn { "Not using mirrors when loading $glideUrl" } + return LoadData(glideUrl, HttpFetcher(httpManager, glideUrl, proxyGetter())) + } + + class Factory( + private val httpManager: HttpManager, + private val proxyGetter: () -> ProxyConfig?, + ) : ModelLoaderFactory { + override fun build(multiFactory: MultiModelLoaderFactory): ModelLoader { + return HttpGlideUrlLoader(httpManager, proxyGetter) + } + + override fun teardown() {} + } + +} diff --git a/download/src/androidTest/kotlin/org/fdroid/download/HttpDownloaderTest.kt b/download/src/androidTest/kotlin/org/fdroid/download/HttpDownloaderTest.kt index 0def799a7..79f2401bb 100644 --- a/download/src/androidTest/kotlin/org/fdroid/download/HttpDownloaderTest.kt +++ b/download/src/androidTest/kotlin/org/fdroid/download/HttpDownloaderTest.kt @@ -27,6 +27,7 @@ import kotlin.test.Test import kotlin.test.assertContentEquals import kotlin.test.assertEquals import kotlin.test.assertTrue +import kotlin.test.fail private const val TOR_SOCKS_PORT = 9050 @@ -71,6 +72,32 @@ class HttpDownloaderTest { httpDownloader.download() assertContentEquals(firstBytes + secondBytes, file.readBytes()) + assertEquals(2, mockEngine.responseHistory.size) + } + + @Test + fun testResumeError() = runSuspend { + val file = folder.newFile() + val firstBytes = Random.nextBytes(1024) + file.writeBytes(firstBytes) + val secondBytes = Random.nextBytes(1024) + val allBytes = firstBytes + secondBytes + + var numRequest = 1 + val mockEngine = MockEngine { + when (numRequest++) { + 1 -> respond("", OK, headers = headersOf(ContentLength, "2048")) + 2 -> respond(allBytes, OK) // not replying with PartialContent + 3 -> respond(allBytes, OK) + else -> fail("Unexpected additional request") + } + } + val httpManager = HttpManager(userAgent, null, httpClientEngineFactory = get(mockEngine)) + val httpDownloader = HttpDownloader(httpManager, downloadRequest, file) + httpDownloader.download() + + assertContentEquals(allBytes, file.readBytes()) + assertEquals(3, mockEngine.responseHistory.size) } @Test diff --git a/download/src/commonMain/kotlin/org/fdroid/download/HttpManager.kt b/download/src/commonMain/kotlin/org/fdroid/download/HttpManager.kt index 77d690c43..b3c3e9fa8 100644 --- a/download/src/commonMain/kotlin/org/fdroid/download/HttpManager.kt +++ b/download/src/commonMain/kotlin/org/fdroid/download/HttpManager.kt @@ -4,10 +4,11 @@ import io.ktor.client.HttpClient import io.ktor.client.call.receive import io.ktor.client.engine.HttpClientEngineFactory import io.ktor.client.engine.ProxyConfig +import io.ktor.client.features.HttpTimeout import io.ktor.client.features.ResponseException -import io.ktor.client.features.ServerResponseException import io.ktor.client.features.UserAgent import io.ktor.client.features.defaultRequest +import io.ktor.client.features.timeout import io.ktor.client.request.get import io.ktor.client.request.head import io.ktor.client.request.header @@ -35,6 +36,7 @@ import io.ktor.utils.io.core.toByteArray import io.ktor.utils.io.readRemaining import io.ktor.utils.io.writeFully import mu.KotlinLogging +import kotlin.coroutines.cancellation.CancellationException import kotlin.jvm.JvmOverloads internal expect fun getHttpClientEngineFactory(): HttpClientEngineFactory<*> @@ -78,6 +80,7 @@ public open class HttpManager @JvmOverloads constructor( install(UserAgent) { agent = userAgent } + install(HttpTimeout) defaultRequest { // add query string parameters if existing parameters?.forEach { (key, value) -> @@ -93,7 +96,7 @@ public open class HttpManager @JvmOverloads constructor( * This is useful for checking if the repository index has changed before downloading it again. * However, due to non-standard ETags on mirrors, change detection is unreliable. */ - suspend fun head(request: DownloadRequest, eTag: String? = null): HeadInfo? { + public suspend fun head(request: DownloadRequest, eTag: String? = null): HeadInfo? { val authString = constructBasicAuthValue(request) val response: HttpResponse = try { mirrorChooser.mirrorRequest(request) { mirror, url -> @@ -102,6 +105,8 @@ public open class HttpManager @JvmOverloads constructor( httpClient.head(url) { // add authorization header from username / password if set if (authString != null) header(Authorization, authString) + // increase connect timeout if using Tor mirror + if (mirror.isOnion()) timeout { connectTimeoutMillis = 10_000 } } } } catch (e: ResponseException) { @@ -117,28 +122,20 @@ public open class HttpManager @JvmOverloads constructor( } @JvmOverloads - suspend fun get( + @Throws(ResponseException::class, NoResumeException::class, CancellationException::class) + public suspend fun get( request: DownloadRequest, skipFirstBytes: Long? = null, receiver: suspend (ByteArray) -> Unit, ) { - val authString = constructBasicAuthValue(request) - mirrorChooser.mirrorRequest(request) { mirror, url -> - resetProxyIfNeeded(request.proxy, mirror) - log.debug { "GET $url" } - httpClient.get(url) { - // add authorization header from username / password if set - if (authString != null) header(Authorization, authString) - // add range header if set - if (skipFirstBytes != null) header(Range, "bytes=${skipFirstBytes}-") - } - }.execute { response -> + get(request, skipFirstBytes).execute { response -> if (skipFirstBytes != null && response.status != PartialContent) { - throw ServerResponseException(response, "expected 206") + throw NoResumeException() } val channel: ByteReadChannel = response.receive() + val limit = 8L * 1024L while (!channel.isClosedForRead) { - val packet = channel.readRemaining(8L * 1024L) + val packet = channel.readRemaining(limit) while (!packet.isEmpty) { receiver(packet.readBytes()) } @@ -146,12 +143,41 @@ public open class HttpManager @JvmOverloads constructor( } } + internal suspend fun get( + request: DownloadRequest, + skipFirstBytes: Long? = null, + ): HttpStatement { + val authString = constructBasicAuthValue(request) + return mirrorChooser.mirrorRequest(request) { mirror, url -> + resetProxyIfNeeded(request.proxy, mirror) + log.debug { "GET $url" } + httpClient.get(url) { + // add authorization header from username / password if set + if (authString != null) header(Authorization, authString) + // increase connect timeout if using Tor mirror + if (mirror.isOnion()) timeout { connectTimeoutMillis = 20_000 } + // add range header if set + if (skipFirstBytes != null) header(Range, "bytes=${skipFirstBytes}-") + } + } + } + + /** + * Returns a [ByteChannel] for streaming download. + */ + internal suspend fun getChannel( + request: DownloadRequest, + skipFirstBytes: Long? = null, + ): ByteReadChannel { + return get(request, skipFirstBytes).receive() + } + /** * Same as [get], but returns all bytes. * Use this only when you are sure that a response will be small. * Thus, this is intentionally visible internally only. + * Does not use [getChannel] so, it gets the [NoResumeException] as in the public API. */ - @JvmOverloads internal suspend fun getBytes(request: DownloadRequest, skipFirstBytes: Long? = null): ByteArray { val channel = ByteChannel() get(request, skipFirstBytes) { bytes -> @@ -176,7 +202,7 @@ public open class HttpManager @JvmOverloads constructor( null } else proxyConfig if (currentProxy != newProxy) { - log.info { "Switching proxy from [$currentProxy] to [$newProxy]"} + log.info { "Switching proxy from [$currentProxy] to [$newProxy]" } httpClient.close() httpClient = getNewHttpClient(newProxy) } diff --git a/download/src/commonMain/kotlin/org/fdroid/download/NoResumeException.kt b/download/src/commonMain/kotlin/org/fdroid/download/NoResumeException.kt new file mode 100644 index 000000000..ce0d5c0d6 --- /dev/null +++ b/download/src/commonMain/kotlin/org/fdroid/download/NoResumeException.kt @@ -0,0 +1,3 @@ +package org.fdroid.download + +public class NoResumeException : Exception() diff --git a/download/src/commonTest/kotlin/org/fdroid/download/HttpManagerTest.kt b/download/src/commonTest/kotlin/org/fdroid/download/HttpManagerTest.kt index b53be458e..83a243565 100644 --- a/download/src/commonTest/kotlin/org/fdroid/download/HttpManagerTest.kt +++ b/download/src/commonTest/kotlin/org/fdroid/download/HttpManagerTest.kt @@ -132,11 +132,9 @@ class HttpManagerTest { assertContentEquals(content.copyOfRange(skipBytes, content.size), httpManager.getBytes(downloadRequest, skipBytes.toLong())) // second request fails, because it responds with OK and full content - val exception = assertFailsWith { + assertFailsWith { httpManager.getBytes(downloadRequest, skipBytes.toLong()) } - val url = mockEngine.requestHistory.last().url - assertEquals("Server error($url: 200 OK. Text: \"expected 206\"", exception.message) } @Test