分离 torrent 连接管理逻辑, helps #1606 #1616
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Him188
reviewed
Jan 30, 2025
app/shared/app-data/src/commonMain/kotlin/domain/torrent/AbstractTorrentServiceConnection.kt
Outdated
Show resolved
Hide resolved
Him188
reviewed
Jan 30, 2025
Open
Him188
reviewed
Feb 2, 2025
| } | ||
| } | ||
|
|
||
| class TestLifecycleOwner : LifecycleOwner { |
There was a problem hiding this comment.
androidx.lifecycle.testing.TestLifecycleOwner
这个是安卓官方的 2.8.7 版本的, 复制过来变成 commonMain 了
| } | ||
| } | ||
|
|
||
| private class TestLifecycle(private val owner: LifecycleOwner) : Lifecycle() { |
Him188
requested changes
Feb 2, 2025
| } | ||
| } | ||
|
|
||
| @RequiresApi(34) |
Comment on lines
+165
to
+192
|
|
||
| /** | ||
| * APP 已进入前台, 此时需要保证服务可用. | ||
| */ | ||
| @CallSuper | ||
| override fun onResume(owner: LifecycleOwner) { | ||
| scope.launch(CoroutineName("TorrentServiceConnection - Lifecycle Resume")) { | ||
| isAtForeground.value = true | ||
| // 服务已经连接了, 不需要再次处理 | ||
| if (isServiceConnected.value) return@launch | ||
|
|
||
| lock.withLock { | ||
| if (isServiceConnected.value) return@launch | ||
|
|
||
| logger.debug { "Service is not started, starting." } | ||
| startServiceWithRetry() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @CallSuper | ||
| override fun onPause(owner: LifecycleOwner) { | ||
| scope.launch(CoroutineName("TorrentServiceConnection - Lifecycle Pause")) { | ||
| lock.withLock { | ||
| isAtForeground.value = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
这个处理有线程安全问题
设想 onResume 被调用, 然后onPause立即被调用分别开了一个协程.
注意到 scope 是并行的 (可以同时执行两个协程).
有三个导致逻辑错误的场景:
scope.launch是没有顺序保证的, 也就是有可能onPause的 launch 会先执行, 然后执行onResume的.onPause协程先执行了, 但是onResume同时也在执行, 有可能onPause抢到了 lock,onResume只能等
isAtForeground 这个 var 非常容易出错. 建议保证只有一个地方会修改这个值. 注意多线程可见性问题.
There was a problem hiding this comment.
我的建议是, 不使用 onPause 等 callback API. 用我之前改的那样 launch 一个协程 collectLatest lifecycle flow 处理, 是没有这些问题的.
Comment on lines
+194
to
+207
| private suspend fun startServiceWithRetry(maxAttempts: Int = Int.MAX_VALUE) { | ||
| var retries = 0 | ||
| while (retries <= maxAttempts) { | ||
| val startResult = startService() | ||
| if (startResult == TorrentServiceConnection.StartResult.FAILED) { | ||
| logger.warn { "[#$retries] Failed to start service." } | ||
| retries++ | ||
| delay(7500) | ||
| } else { | ||
| return | ||
| } | ||
| } | ||
| logger.error { "Failed to start service after $maxAttempts retries." } | ||
| } |
Comment on lines
+220
to
+226
| override suspend fun getBinder(): T { | ||
| // track cancellation of [scope] | ||
| scope.async { | ||
| isServiceConnected.first { it } | ||
| }.await() | ||
| return binderDeferred.await() | ||
| } |
There was a problem hiding this comment.
not thread safe. isServiceConnected.first { it } 访问 binderDeferred.await() 和 isServiceConnected.first { it } 未上锁. 上一行检测到 isServiceConnected 为 true, 但下一行可能 binderDeferred 变了.
| * 服务已连接, 服务通信对象一定可用. | ||
| * 无论当前 [Lifecycle] 什么状态都要应用新的 [binder]. | ||
| */ | ||
| protected fun onServiceConnected(binder: T) { |
There was a problem hiding this comment.
not safe.
继承者可能会在不适合的时机调用 onServiceConnected. 而此函数没有检查状态是否合理.
继承者不调用此函数的情况也没处理.
| * 若返回了 [StartResult.STARTED] 或 [StartResult.FAILED], | ||
| * [connected] 将在未来变为 `true`, 届时 [getBinder] 将会立刻返回服务通信接口. | ||
| */ | ||
| suspend fun startService(): StartResult |
| /** | ||
| * torrent 服务与 APP 通信接口. | ||
| */ | ||
| interface TorrentServiceConnection<T : Any> { |
| // 请求 wake lock, 如果在 app 中息屏可以保证 service 正常跑 [acquireWakeLockIntent] 分钟. | ||
| context.startService(acquireWakeLockIntent) | ||
| } catch (ex: IllegalStateException) { | ||
| // 大概率是 ServiceStartForegroundException, 服务已经终止了, 不需要再请求 wakelock. |
There was a problem hiding this comment.
catch ForegroundServiceStartNotAllowedException explicitly
Comment on lines
+55
to
+60
| private val startupIntentFilter by lazy { IntentFilter(AniTorrentService.INTENT_STARTUP) } | ||
| private val acquireWakeLockIntent by lazy { | ||
| Intent(context, AniTorrentService.actualServiceClass).apply { | ||
| putExtra("acquireWakeLock", 1.minutes.inWholeMilliseconds) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is lazy really needed here?
Lazy introduces synchornization overhead (which is on main thread).
Comment on lines
+128
to
+133
| ContextCompat.registerReceiver( | ||
| context, | ||
| timeExceedLimitReceiver, | ||
| timeExceedLimitIntentFilter, | ||
| ContextCompat.RECEIVER_NOT_EXPORTED, | ||
| ) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.