Feature/graphql download queue subscription send only updates (#1011)

* Emit only download changes instead of full status The download subscription emitted the full download status, which, depending on how big the queue was, took forever because the graphql subscription does not support data loader batching, causing it to run into the n+1 problem * Rename "DownloadManager#status" to "DownloadManager#updates" * Add initial queue to download subscription type Adds the current queue at the time of sending the initial message. This field is null for all following messages after the initial one * Optionally limit and omit download updates To prevent the n+1 dataloader issue, the max number of updates included in the download subscription can be limited. This way, the problem will be circumvented and instead, the latest download status should be (re-)fetched via the download status query, which does not run into this problem. * Formatting
Suwayomi · Nov 14, 2024 · 168b76c · 168b76c
1 parent f5680c6
commit 168b76c
Show file tree

Hide file tree

Showing 9 changed files with 207 additions and 30 deletions.
diff --git a/server/src/main/kotlin/suwayomi/tachidesk/graphql/mutations/DownloadMutation.kt b/server/src/main/kotlin/suwayomi/tachidesk/graphql/mutations/DownloadMutation.kt
@@ -10,6 +10,7 @@ import suwayomi.tachidesk.graphql.types.ChapterType
 import suwayomi.tachidesk.graphql.types.DownloadStatus
 import suwayomi.tachidesk.manga.impl.Chapter
 import suwayomi.tachidesk.manga.impl.download.DownloadManager
+import suwayomi.tachidesk.manga.impl.download.model.DownloadUpdateType.DEQUEUED
 import suwayomi.tachidesk.manga.impl.download.model.Status
 import suwayomi.tachidesk.manga.model.table.ChapterTable
 import suwayomi.tachidesk.server.JavalinSetup.future
@@ -94,7 +95,12 @@ class DownloadMutation {
                     clientMutationId = clientMutationId,
                     downloadStatus =
                         withTimeout(30.seconds) {
-                            DownloadStatus(DownloadManager.status.first { it.queue.any { it.chapter.id in chapters } })
+                            DownloadStatus(
+                                DownloadManager.updates
+                                    .first {
+                                        DownloadManager.getStatus().queue.any { it.chapter.id in chapters }
+                                    }.let { DownloadManager.getStatus() },
+                            )
                         },
                 )
             }
@@ -122,7 +128,11 @@ class DownloadMutation {
                     clientMutationId = clientMutationId,
                     downloadStatus =
                         withTimeout(30.seconds) {
-                            DownloadStatus(DownloadManager.status.first { it.queue.any { it.chapter.id == chapter } })
+                            DownloadStatus(
+                                DownloadManager.updates
+                                    .first { it.updates.any { it.downloadChapter.chapter.id == chapter } }
+                                    .let { DownloadManager.getStatus() },
+                            )
                         },
                 )
             }
@@ -152,7 +162,14 @@ class DownloadMutation {
                     clientMutationId = clientMutationId,
                     downloadStatus =
                         withTimeout(30.seconds) {
-                            DownloadStatus(DownloadManager.status.first { it.queue.none { it.chapter.id in chapters } })
+                            DownloadStatus(
+                                DownloadManager.updates
+                                    .first {
+                                        it.updates.none {
+                                            it.downloadChapter.chapter.id in chapters && it.type != DEQUEUED
+                                        }
+                                    }.let { DownloadManager.getStatus() },
+                            )
                         },
                 )
             }
@@ -180,7 +197,14 @@ class DownloadMutation {
                     clientMutationId = clientMutationId,
                     downloadStatus =
                         withTimeout(30.seconds) {
-                            DownloadStatus(DownloadManager.status.first { it.queue.none { it.chapter.id == chapter } })
+                            DownloadStatus(
+                                DownloadManager.updates
+                                    .first {
+                                        it.updates.none {
+                                            it.downloadChapter.chapter.id == chapter && it.type != DEQUEUED
+                                        }
+                                    }.let { DownloadManager.getStatus() },
+                            )
                         },
                 )
             }
@@ -206,7 +230,9 @@ class DownloadMutation {
                     downloadStatus =
                         withTimeout(30.seconds) {
                             DownloadStatus(
-                                DownloadManager.status.first { it.status == Status.Started },
+                                DownloadManager.updates
+                                    .first { it.status == Status.Started }
+                                    .let { DownloadManager.getStatus() },
                             )
                         },
                 )
@@ -232,7 +258,9 @@ class DownloadMutation {
                     downloadStatus =
                         withTimeout(30.seconds) {
                             DownloadStatus(
-                                DownloadManager.status.first { it.status == Status.Stopped },
+                                DownloadManager.updates
+                                    .first { it.status == Status.Stopped }
+                                    .let { DownloadManager.getStatus() },
                             )
                         },
                 )
@@ -258,7 +286,9 @@ class DownloadMutation {
                     downloadStatus =
                         withTimeout(30.seconds) {
                             DownloadStatus(
-                                DownloadManager.status.first { it.status == Status.Stopped && it.queue.isEmpty() },
+                                DownloadManager.updates
+                                    .first { it.status == Status.Stopped }
+                                    .let { DownloadManager.getStatus() },
                             )
                         },
                 )
@@ -288,7 +318,9 @@ class DownloadMutation {
                     downloadStatus =
                         withTimeout(30.seconds) {
                             DownloadStatus(
-                                DownloadManager.status.first { it.queue.indexOfFirst { it.chapter.id == chapter } <= to },
+                                DownloadManager.updates
+                                    .first { it.updates.indexOfFirst { it.downloadChapter.chapter.id == chapter } <= to }
+                                    .let { DownloadManager.getStatus() },
                             )
                         },
                 )

diff --git a/server/src/main/kotlin/suwayomi/tachidesk/graphql/queries/DownloadQuery.kt b/server/src/main/kotlin/suwayomi/tachidesk/graphql/queries/DownloadQuery.kt
@@ -1,6 +1,5 @@
 package suwayomi.tachidesk.graphql.queries
 
-import kotlinx.coroutines.flow.first
 import suwayomi.tachidesk.graphql.types.DownloadStatus
 import suwayomi.tachidesk.manga.impl.download.DownloadManager
 import suwayomi.tachidesk.server.JavalinSetup.future
@@ -9,6 +8,6 @@ import java.util.concurrent.CompletableFuture
 class DownloadQuery {
     fun downloadStatus(): CompletableFuture<DownloadStatus> =
         future {
-            DownloadStatus(DownloadManager.status.first())
+            DownloadStatus(DownloadManager.getStatus())
         }
 }
diff --git a/server/src/main/kotlin/suwayomi/tachidesk/graphql/subscriptions/DownloadSubscription.kt b/server/src/main/kotlin/suwayomi/tachidesk/graphql/subscriptions/DownloadSubscription.kt
@@ -7,14 +7,54 @@
 
 package suwayomi.tachidesk.graphql.subscriptions
 
+import com.expediagroup.graphql.generator.annotations.GraphQLDeprecated
+import com.expediagroup.graphql.generator.annotations.GraphQLDescription
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.map
 import suwayomi.tachidesk.graphql.types.DownloadStatus
+import suwayomi.tachidesk.graphql.types.DownloadUpdates
 import suwayomi.tachidesk.manga.impl.download.DownloadManager
 
 class DownloadSubscription {
+    @GraphQLDeprecated("Replaced width downloadStatusChanged", ReplaceWith("downloadStatusChanged(input)"))
     fun downloadChanged(): Flow<DownloadStatus> =
         DownloadManager.status.map { downloadStatus ->
             DownloadStatus(downloadStatus)
         }
+
+    data class DownloadChangedInput(
+        @GraphQLDescription(
+            "Sets a max number of updates that can be contained in a download update message." +
+                "Everything above this limit will be omitted and the \"downloadStatus\" should be re-fetched via the " +
+                "corresponding query. Due to the graphql subscription execution strategy not supporting batching for data loaders, " +
+                "the data loaders run into the n+1 problem, which can cause the server to get unresponsive until the status " +
+                "update has been handled. This is an issue e.g. when mass en- or dequeuing downloads.",
+        )
+        val maxUpdates: Int?,
+    )
+
+    fun downloadStatusChanged(input: DownloadChangedInput): Flow<DownloadUpdates> {
+        val omitUpdates = input.maxUpdates != null
+        val maxUpdates = input.maxUpdates ?: 50
+
+        return DownloadManager.updates.map { downloadUpdates ->
+            val omittedUpdates = omitUpdates && downloadUpdates.updates.size > maxUpdates
+
+            // the graphql subscription execution strategy does not support data loader batching which causes the n+1 problem,
+            // thus, too many updates (e.g. on mass enqueue or dequeue) causes unresponsiveness of the server until the
+            // update has been handled
+            val actualDownloadUpdates =
+                if (omittedUpdates) {
+                    suwayomi.tachidesk.manga.impl.download.model.DownloadUpdates(
+                        downloadUpdates.status,
+                        downloadUpdates.updates.subList(0, maxUpdates),
+                        downloadUpdates.initial,
+                    )
+                } else {
+                    downloadUpdates
+                }
+
+            DownloadUpdates(actualDownloadUpdates, omittedUpdates)
+        }
+    }
 }
diff --git a/server/src/main/kotlin/suwayomi/tachidesk/graphql/types/DownloadType.kt b/server/src/main/kotlin/suwayomi/tachidesk/graphql/types/DownloadType.kt
@@ -7,6 +7,7 @@
 
 package suwayomi.tachidesk.graphql.types
 
+import com.expediagroup.graphql.generator.annotations.GraphQLDescription
 import com.expediagroup.graphql.generator.annotations.GraphQLIgnore
 import com.expediagroup.graphql.server.extensions.getValueFromDataLoader
 import graphql.schema.DataFetchingEnvironment
@@ -18,6 +19,9 @@ import suwayomi.tachidesk.graphql.server.primitives.PageInfo
 import suwayomi.tachidesk.graphql.types.DownloadState.FINISHED
 import suwayomi.tachidesk.manga.impl.download.model.DownloadChapter
 import suwayomi.tachidesk.manga.impl.download.model.DownloadStatus
+import suwayomi.tachidesk.manga.impl.download.model.DownloadUpdate
+import suwayomi.tachidesk.manga.impl.download.model.DownloadUpdateType
+import suwayomi.tachidesk.manga.impl.download.model.DownloadUpdates
 import suwayomi.tachidesk.manga.impl.download.model.Status
 import java.util.concurrent.CompletableFuture
 import suwayomi.tachidesk.manga.impl.download.model.DownloadState as OtherDownloadState
@@ -35,6 +39,28 @@ data class DownloadStatus(
     )
 }
 
+data class DownloadUpdates(
+    val state: DownloaderState,
+    val updates: List<suwayomi.tachidesk.graphql.types.DownloadUpdate>,
+    @GraphQLDescription("The current download queue at the time of sending initial message. Is null for all following messages")
+    val initial: List<DownloadType>?,
+    @GraphQLDescription(
+        "Indicates whether updates have been omitted based on the \"maxUpdates\" subscription variable. " +
+            "In case updates have been omitted, the \"downloadStatus\" query should be re-fetched.",
+    )
+    val omittedUpdates: Boolean,
+) {
+    constructor(downloadUpdates: DownloadUpdates, omittedUpdates: Boolean) : this(
+        when (downloadUpdates.status) {
+            Status.Stopped -> DownloaderState.STOPPED
+            Status.Started -> DownloaderState.STARTED
+        },
+        downloadUpdates.updates.map { DownloadUpdate(it) },
+        downloadUpdates.initial?.map { DownloadType(it) },
+        omittedUpdates,
+    )
+}
+
 class DownloadType(
     @get:GraphQLIgnore
     val chapterId: Int,
@@ -43,6 +69,7 @@ class DownloadType(
     val state: DownloadState,
     val progress: Float,
     val tries: Int,
+    val position: Int,
 ) : Node {
     constructor(downloadChapter: DownloadChapter) : this(
         downloadChapter.chapter.id,
@@ -55,6 +82,7 @@ class DownloadType(
         },
         downloadChapter.progress,
         downloadChapter.tries,
+        downloadChapter.position,
     )
 
     fun manga(dataFetchingEnvironment: DataFetchingEnvironment): CompletableFuture<MangaType> {
@@ -76,6 +104,16 @@ class DownloadType(
     }
 }
 
+class DownloadUpdate(
+    val type: DownloadUpdateType,
+    val download: DownloadType,
+) : Node {
+    constructor(downloadUpdate: DownloadUpdate) : this(
+        downloadUpdate.type,
+        DownloadType(downloadUpdate.downloadChapter),
+    )
+}
+
 enum class DownloadState {
     QUEUED,
     DOWNLOADING,