di: stop regenerating the Koin music service module at runtime

The music service module was unloaded and reloaded by MusicServiceFactory on
every library access state change, which forced MusicServiceFactory and
ImageLoaderProvider to inject the Koin instance itself and the fetcher
factories to resolve the SubsonicAPIClient lazily from a volatile module.

- MusicServiceFactory now owns the swappable delegates directly: it builds
  OfflineMusicService or CachedMusicService(RESTMusicService(client)) for the
  current access state, exposes apiClient() alongside getMusicService(), and
  keeps resetMusicService() as the explicit hook for server property edits
- the delegate builders are constructor-injectable, keeping the switch logic
  testable without the REST stack
- ImageLoaderProvider takes a () -> SubsonicAPIClient provider and rebuilds
  the loader when the client instance changes, replacing the ServerID Koin key
- CoverArtFetcher/AvatarFetcher factories resolve the client through the
  factory instead of the module
- musicServiceModule is deleted; the DI graph is static and no module is
  ever unloaded at runtime
This commit is contained in:
birdbird committed 2026-10-01 13:33:50 +02:00
1 parent bae723595a
commit 5501c3294b
9 files changed
+332 -162

No files matched your search

@@ -25,13 +25,13 @@ import org.moire.ultrasonic.di.appPermanentStorage
import org.moire.ultrasonic.di.applicationModule
import org.moire.ultrasonic.di.baseNetworkModule
import org.moire.ultrasonic.di.mediaPlayerModule
import org.moire.ultrasonic.di.musicServiceModule
import org.moire.ultrasonic.imageloader.AvatarFetcher
import org.moire.ultrasonic.imageloader.AvatarKeyer
import org.moire.ultrasonic.imageloader.CoverArtFetcher
import org.moire.ultrasonic.imageloader.CoverArtKeyer
import org.moire.ultrasonic.log.FileLoggerTree
import org.moire.ultrasonic.log.TimberKoinLogger
import org.moire.ultrasonic.service.MusicServiceFactory
import org.moire.ultrasonic.util.CoroutinePatterns
import org.moire.ultrasonic.util.FileUtil
import org.moire.ultrasonic.util.Settings
@@ -93,11 +93,11 @@ class UApp :
override fun newImageLoader(context: Context): ImageLoader = ImageLoader.Builder(context)
.components {
// The client is resolved lazily when a fetcher is created: the SubsonicAPIClient
// is regenerated whenever the active server changes.
add(CoverArtFetcher.Factory { get() })
// The client is resolved lazily when a fetcher is created: the factory
// rebuilds the SubsonicAPIClient whenever the active server changes.
add(CoverArtFetcher.Factory { get<MusicServiceFactory>().apiClient() })
add(CoverArtKeyer())
add(AvatarFetcher.Factory { get() })
add(AvatarFetcher.Factory { get<MusicServiceFactory>().apiClient() })
add(AvatarKeyer())
}
.memoryCache(
@@ -138,7 +138,6 @@ class UApp :
applicationModule,
appPermanentStorage,
baseNetworkModule,
musicServiceModule,
mediaPlayerModule
)
}
@@ -1,9 +1,10 @@
package org.moire.ultrasonic.di
import org.koin.core.Koin
import okhttp3.logging.HttpLoggingInterceptor
import org.koin.dsl.module
import org.moire.ultrasonic.adapters.PopupMenuFactory
import org.moire.ultrasonic.data.ActiveServerProvider
import org.moire.ultrasonic.log.TimberOkHttpLogger
import org.moire.ultrasonic.service.MusicServiceFactory
import org.moire.ultrasonic.subsonic.ImageLoaderProvider
import org.moire.ultrasonic.subsonic.VideoPlayer
@@ -18,15 +19,13 @@ import org.moire.ultrasonic.util.DownloadUtil
val applicationModule = module {
single { ActiveServerProvider(get()) }
// Must NOT live in musicServiceModule: that module is unloaded/reloaded by the
// factory itself whenever the library access state changes.
single { MusicServiceFactory(get(), getKoin()) }
single<HttpLoggingInterceptor.Logger> { TimberOkHttpLogger() }
single { MusicServiceFactory(get(), get()) }
// The ImageLoaderProvider resolves the SubsonicAPIClient and the ServerID from the
// music service module at request time, because that module is regenerated whenever
// the active server changes. It therefore receives the Koin instance itself instead
// of a fixed snapshot of its definitions.
single { ImageLoaderProvider(getKoin()) }
// The ImageLoaderProvider resolves the SubsonicAPIClient per request: the factory
// rebuilds it whenever the active server or its properties change, and the provider
// rebuilds the loader when the client instance changes.
single { ImageLoaderProvider { get<MusicServiceFactory>().apiClient() } }
single { PopupMenuFactory(get()) }
single { CacheCleaner(get(), get()) }
@@ -1,68 +0,0 @@
@file:JvmName("MusicServiceModule")
package org.moire.ultrasonic.di
import kotlin.math.abs
import okhttp3.logging.HttpLoggingInterceptor
import org.koin.core.qualifier.named
import org.koin.dsl.module
import org.moire.ultrasonic.BuildConfig
import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
import org.moire.ultrasonic.api.subsonic.SubsonicAPIVersions
import org.moire.ultrasonic.api.subsonic.SubsonicClientConfiguration
import org.moire.ultrasonic.data.ActiveServerProvider
import org.moire.ultrasonic.log.TimberOkHttpLogger
import org.moire.ultrasonic.service.CachedMusicService
import org.moire.ultrasonic.service.MusicService
import org.moire.ultrasonic.service.OfflineMusicService
import org.moire.ultrasonic.service.RESTMusicService
import org.moire.ultrasonic.util.Constants
/**
* This Koin module contains the registration of classes related to the Music Services
*/
internal const val ONLINE_MUSIC_SERVICE = "OnlineMusicService"
internal const val OFFLINE_MUSIC_SERVICE = "OfflineMusicService"
val musicServiceModule = module {
single(named("ServerInstance")) {
return@single get<ActiveServerProvider>().getSelectedServerId()
}
single(named("ServerID")) {
val serverInstance = get<Int>(named("ServerInstance"))
val serverUrl = get<ActiveServerProvider>().getActiveServer().url
return@single abs("$serverUrl$serverInstance".hashCode()).toString()
}
single {
val server = get<ActiveServerProvider>().getActiveServer()
return@single SubsonicClientConfiguration(
baseUrl = server.url,
username = server.userName,
password = server.password,
minimalProtocolVersion = SubsonicAPIVersions.getClosestKnownClientApiVersion(
server.minimumApiVersion
?: Constants.REST_PROTOCOL_VERSION
),
clientID = Constants.REST_CLIENT_ID,
allowSelfSignedCertificate = server.allowSelfSignedCertificate,
forcePlainTextPassword = server.forcePlainTextPassword,
debug = BuildConfig.DEBUG,
isRealProtocolVersion = server.minimumApiVersion != null
)
}
single<HttpLoggingInterceptor.Logger> { TimberOkHttpLogger() }
single { SubsonicAPIClient(get(), get()) }
single<MusicService>(named(ONLINE_MUSIC_SERVICE)) {
CachedMusicService(RESTMusicService(get(), get()), get())
}
single<MusicService>(named(OFFLINE_MUSIC_SERVICE)) {
OfflineMusicService(get())
}
}
@@ -18,43 +18,118 @@
*/
package org.moire.ultrasonic.service
import org.koin.core.Koin
import org.koin.core.qualifier.named
import okhttp3.logging.HttpLoggingInterceptor
import org.moire.ultrasonic.BuildConfig
import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
import org.moire.ultrasonic.api.subsonic.SubsonicAPIVersions
import org.moire.ultrasonic.api.subsonic.SubsonicClientConfiguration
import org.moire.ultrasonic.data.ActiveServerProvider
import org.moire.ultrasonic.data.LibraryAccessState
import org.moire.ultrasonic.di.OFFLINE_MUSIC_SERVICE
import org.moire.ultrasonic.di.ONLINE_MUSIC_SERVICE
import org.moire.ultrasonic.di.musicServiceModule
import org.moire.ultrasonic.util.Constants
import timber.log.Timber
/*
* TODO: When the music service module is regenerated, a large number of classes are completely
* newly instantiated, which take quite a bit of time.
*/
/**
* Provides the [MusicService] matching the current library access state.
* Provides the [MusicService] matching the current library access state, and the
* [SubsonicAPIClient] behind it (e.g. for image loading).
*
* The Subsonic client snapshots the server settings when its Koin module is loaded, so
* after an access state change (server switch, offline transition, ...) the module must
* be regenerated before the next service is handed out. Instead of relying on an
* external reset call, [getMusicService] checks for itself whether the module is still
* valid for the current state, and regenerates it if needed.
* The factory owns the swappable service instances instead of relying on a Koin module
* that is unloaded and reloaded at runtime: the DI graph stays static, and consumers get
* fresh delegates by calling [getMusicService] (or [apiClient]) per operation. Only the
* transition dimensions of the access state are compared; server property edits
* (invalidateCache) intentionally do not rebuild the delegate - call [resetMusicService]
* after changing server properties to force one.
*/
class MusicServiceFactory(
private val activeServerProvider: ActiveServerProvider,
// The Koin instance is injected instead of using the KoinComponent service locator,
// because this factory is the one component that must resolve from a Koin module
// that is regenerated at runtime (see below).
private val koin: Koin
private val okLogger: HttpLoggingInterceptor.Logger,
// The delegate builders are injectable so the switch logic can be tested without
// touching the REST stack.
private val onlineServiceFactory: (SubsonicAPIClient) -> MusicService = { apiClient ->
CachedMusicService(
RESTMusicService(apiClient, activeServerProvider),
activeServerProvider
)
},
private val offlineServiceFactory: () -> MusicService = {
OfflineMusicService(activeServerProvider)
}
) {
// Identifies the access state the module was last loaded for. Only the transition
// dimensions are compared: server property edits (invalidateCache) intentionally do
// not regenerate the module.
// Identifies the access state the delegate was last built for. Only the transition
// dimensions are compared: server property edits intentionally do not rebuild.
private var loadedStateKey: Triple<Int, Boolean, LibraryAccessState.AutomaticOfflineReason?>? =
null
@Volatile
private var currentDelegate: MusicService? = null
@Volatile
private var currentApiClient: SubsonicAPIClient? = null
/**
* The [MusicService] for the current access state. Call it per operation instead of
* caching the result: the returned instance is swapped after transitions.
*/
fun getMusicService(): MusicService {
ensureMusicServiceUpToDate()
return requireNotNull(currentDelegate) { "MusicService was not built" }
}
/**
* The [SubsonicAPIClient] behind the effective server, e.g. for image loading. In
* offline mode it points at the offline pseudo-server, matching the previous behavior
* of the regenerated Koin module.
*/
fun apiClient(): SubsonicAPIClient {
ensureMusicServiceUpToDate()
return requireNotNull(currentApiClient) { "The API client was not built" }
}
/**
* Forces the delegate to be rebuilt on the next access, e.g. after the properties of
* the active server were edited: the transition dimensions of the access state do not
* change for those, so the key comparison alone would keep serving the old client.
*/
@Synchronized
fun resetMusicService() {
loadedStateKey = null
}
// Rebuild the delegate if the access state changed since the last build.
@Synchronized
private fun ensureMusicServiceUpToDate() {
val key = accessStateKey
if (key == loadedStateKey && currentDelegate != null) return
buildMusicService()
}
@Synchronized
private fun buildMusicService() {
Timber.i("Building the MusicService for the current access state")
val server = activeServerProvider.getActiveServer()
val configuration = SubsonicClientConfiguration(
baseUrl = server.url,
username = server.userName,
password = server.password,
minimalProtocolVersion = SubsonicAPIVersions.getClosestKnownClientApiVersion(
server.minimumApiVersion ?: Constants.REST_PROTOCOL_VERSION
),
clientID = Constants.REST_CLIENT_ID,
allowSelfSignedCertificate = server.allowSelfSignedCertificate,
forcePlainTextPassword = server.forcePlainTextPassword,
debug = BuildConfig.DEBUG,
isRealProtocolVersion = server.minimumApiVersion != null
)
val apiClient = SubsonicAPIClient(configuration, okLogger)
currentApiClient = apiClient
currentDelegate = if (activeServerProvider.isOffline()) {
offlineServiceFactory()
} else {
onlineServiceFactory(apiClient)
}
loadedStateKey = accessStateKey
}
private val accessStateKey: Triple<Int, Boolean, LibraryAccessState.AutomaticOfflineReason?>
get() {
val state = activeServerProvider.getLibraryAccessState()
@@ -64,35 +139,4 @@ class MusicServiceFactory(
state.automaticOfflineReason
)
}
fun getMusicService(): MusicService {
ensureMusicServiceUpToDate()
return if (activeServerProvider.isOffline()) {
koin.get(named(OFFLINE_MUSIC_SERVICE))
} else {
koin.get(named(ONLINE_MUSIC_SERVICE))
}
}
/**
* Regenerates the music service module immediately, e.g. after the properties of
* the active server were edited. The next [getMusicService] call then serves the
* updated services.
*/
@Synchronized
fun resetMusicService() {
Timber.i("Regenerating Koin Music Service Module")
koin.unloadModules(listOf(musicServiceModule))
koin.loadModules(listOf(musicServiceModule))
loadedStateKey = accessStateKey
}
// Regenerate the module if the access state changed since the last load.
@Synchronized
private fun ensureMusicServiceUpToDate() {
val currentKey = accessStateKey
if (currentKey == loadedStateKey) return
resetMusicService()
}
}
@@ -37,9 +37,7 @@ internal class PlaybackLoadErrorPolicy(
private val retryDelaysMs: LongArray = DEFAULT_RETRY_DELAYS_MS
) : DefaultLoadErrorHandlingPolicy() {
override fun getRetryDelayMsFor(
loadErrorInfo: LoadErrorHandlingPolicy.LoadErrorInfo
): Long {
override fun getRetryDelayMsFor(loadErrorInfo: LoadErrorHandlingPolicy.LoadErrorInfo): Long {
if (loadErrorInfo.exception.hasTransientConnectivityCause()) {
val attempt = loadErrorInfo.errorCount - 1
return if (attempt < retryDelaysMs.size) {
@@ -53,12 +51,14 @@ internal class PlaybackLoadErrorPolicy(
override fun getMinimumLoadableRetryCount(dataType: Int): Int = retryDelaysMs.size
private fun IOException.hasTransientConnectivityCause(): Boolean =
causeChain().any { it is SocketTimeoutException || it is ConnectException ||
it is SocketException || it is UnknownHostException }
private fun IOException.hasTransientConnectivityCause(): Boolean = causeChain().any {
it is SocketTimeoutException || it is ConnectException ||
it is SocketException || it is UnknownHostException
}
companion object {
// 1s, 2s, 4s, then capped 10s steps: roughly 45s of total retry budget.
val DEFAULT_RETRY_DELAYS_MS = longArrayOf(1_000, 2_000, 4_000, 8_000, 10_000, 10_000, 10_000)
val DEFAULT_RETRY_DELAYS_MS =
longArrayOf(1_000, 2_000, 4_000, 8_000, 10_000, 10_000, 10_000)
}
}
@@ -12,9 +12,8 @@ import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import org.koin.core.Koin
import org.koin.core.qualifier.named
import org.moire.ultrasonic.R
import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
import org.moire.ultrasonic.app.UApp
import org.moire.ultrasonic.imageloader.ImageLoader
import org.moire.ultrasonic.imageloader.ImageLoaderConfig
@@ -27,18 +26,13 @@ import timber.log.Timber
* Handles the lifetime of the Image Loader
*/
class ImageLoaderProvider(
// The Koin instance is injected instead of using the KoinComponent service locator:
// the SubsonicAPIClient and the ServerID live in the music service module which is
// regenerated on every server switch, so they must be re-resolved lazily.
private val koin: Koin
// The client is resolved lazily at request time: the MusicServiceFactory rebuilds
// the SubsonicAPIClient whenever the active server or its properties change, and the
// loader is rebuilt when the client instance changes.
private val clientProvider: () -> SubsonicAPIClient
) : CoroutineScope by CoroutinePatterns.loggingScope(Dispatchers.IO) {
private var imageLoader: ImageLoader? = null
private var serverID: String = koin.get(named("ServerID"))
@Synchronized
fun clearImageLoader() {
imageLoader = null
}
private var lastApiClient: SubsonicAPIClient? = null
init {
Timber.d("Prepping Loader")
@@ -51,10 +45,10 @@ class ImageLoaderProvider(
@Synchronized
fun getImageLoader(): ImageLoader {
// We need to generate a new ImageLoader if the server has changed...
val currentID = koin.get<String>(named("ServerID"))
if (imageLoader == null || currentID != serverID) {
imageLoader = ImageLoader(UApp.applicationContext(), koin.get(), config)
serverID = currentID
val client = clientProvider()
if (imageLoader == null || client !== lastApiClient) {
imageLoader = ImageLoader(UApp.applicationContext(), client, config)
lastApiClient = client
launch {
FileUtil.ensureAlbumArtDirectory()
@@ -0,0 +1,202 @@
/*
* MusicServiceFactoryTest.kt
* Copyright (C) 2009-2026 Ultrasonic developers
*
* Distributed under terms of the GNU GPLv3 license.
*/
package org.moire.ultrasonic.service
import android.content.Context
import androidx.test.core.app.ApplicationProvider
import okhttp3.logging.HttpLoggingInterceptor
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldNotBeEqualTo
import org.amshove.kluent.shouldNotBeNull
import org.junit.After
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.reset
import org.mockito.kotlin.any
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
import org.moire.ultrasonic.app.UApp
import org.moire.ultrasonic.data.ActiveServerProvider
import org.moire.ultrasonic.data.LibraryAccessState
import org.moire.ultrasonic.data.ServerSetting
import org.robolectric.RobolectricTestRunner
/**
* Verifies the switch logic of the long-lived [MusicServiceFactory]: the delegate is
* rebuilt when the transition dimensions of the access state change or when
* [MusicServiceFactory.resetMusicService] forces it, and is kept when neither applies.
*
* Robolectric is required because mocking ActiveServerProvider initializes its companion
* object, which reads Android settings and resources.
*/
@RunWith(RobolectricTestRunner::class)
class MusicServiceFactoryTest {
companion object {
private val activeServerProvider: ActiveServerProvider by lazy { mock() }
}
private var builtApiClients = mutableListOf<SubsonicAPIClient>()
private var builtDelegates = mutableListOf<MusicService>()
private val factory = MusicServiceFactory(
activeServerProvider,
okLogger = mock<HttpLoggingInterceptor.Logger>(),
onlineServiceFactory = { apiClient ->
builtApiClients += apiClient
builtDelegates += mock<MusicService>()
builtDelegates.last()
},
offlineServiceFactory = {
builtDelegates += mock<MusicService>()
builtDelegates.last()
}
)
@Before
fun setUp() {
val context = ApplicationProvider.getApplicationContext<Context>()
val appContext: Context = mock()
whenever(appContext.getString(any())).thenAnswer { it.arguments[0].toString() }
whenever(appContext.applicationContext).thenReturn(appContext)
whenever(appContext.packageName).thenReturn(context.packageName)
whenever(appContext.getSharedPreferences(any(), any())).thenAnswer {
context.getSharedPreferences(it.arguments[0] as String, it.arguments[1] as Int)
}
val app: UApp = mock()
whenever(app.applicationContext).thenReturn(appContext)
UApp.instance = app
reset(activeServerProvider)
builtApiClients.clear()
builtDelegates.clear()
whenever(activeServerProvider.getSelectedServerId()).thenReturn(5)
}
@After
fun tearDown() {
UApp.instance = null
}
private fun stubState(
selectedServerId: Int = 5,
explicitOffline: Boolean = false,
reason: LibraryAccessState.AutomaticOfflineReason? = null,
serverUrl: String = "https://example.com"
) {
val server = ServerSetting(
id = selectedServerId,
index = 1,
name = "server",
url = serverUrl,
userName = "user",
password = "pass",
jukeboxByDefault = false,
allowSelfSignedCertificate = false,
forcePlainTextPassword = false,
musicFolderId = "",
minimumApiVersion = null
)
val state = LibraryAccessState(selectedServerId, explicitOffline, reason, server)
whenever(activeServerProvider.getLibraryAccessState()).thenReturn(state)
whenever(activeServerProvider.isOffline()).thenReturn(
explicitOffline || reason != null || selectedServerId < 1
)
whenever(activeServerProvider.getActiveServer()).thenReturn(server)
}
@Test
fun `unchanged access state keeps the delegate`() {
stubState()
val first = factory.getMusicService()
val second = factory.getMusicService()
second shouldBeEqualTo first
builtDelegates.size shouldBeEqualTo 1
}
@Test
fun `a changed selection rebuilds the delegate`() {
stubState()
factory.getMusicService()
stubState(selectedServerId = 9)
val second = factory.getMusicService()
second shouldNotBeEqualTo builtDelegates.first()
builtDelegates.size shouldBeEqualTo 2
}
@Test
fun `entering automatic offline switches to the offline delegate`() {
stubState()
factory.getMusicService()
stubState(reason = LibraryAccessState.AutomaticOfflineReason.TIMEOUT)
val offline = factory.getMusicService()
offline shouldNotBeEqualTo builtDelegates.first()
builtDelegates.size shouldBeEqualTo 2
}
@Test
fun `url edits keep the client until resetMusicService forces a rebuild`() {
stubState(serverUrl = "https://one.example.com")
val firstClient = factory.apiClient()
// Same transition dimensions: an edited URL alone must not rebuild the delegate.
// EditServerFragment calls resetMusicService explicitly after property edits.
stubState(serverUrl = "https://two.example.com")
val staleClient = factory.apiClient()
staleClient shouldBeEqualTo firstClient
factory.resetMusicService()
val rebuiltClient = factory.apiClient()
rebuiltClient shouldNotBeEqualTo firstClient
builtApiClients.size shouldBeEqualTo 2
}
@Test
fun `resetMusicService forces a rebuild without a state change`() {
stubState()
factory.getMusicService()
factory.resetMusicService()
val second = factory.getMusicService()
second shouldNotBeEqualTo builtDelegates.first()
builtDelegates.size shouldBeEqualTo 2
}
@Test
fun `silent refresh keeps the delegate but resets nothing`() {
stubState()
val first = factory.getMusicService()
// invalidateCache-style refresh: same transition dimensions.
stubState()
val second = factory.getMusicService()
second shouldBeEqualTo first
builtDelegates.size shouldBeEqualTo 1
}
@Test
fun `offline state exposes a client pointing at the offline server`() {
stubState(explicitOffline = true)
val client = factory.apiClient()
client.shouldNotBeNull()
}
}
@@ -11,8 +11,8 @@ import android.net.Uri
import androidx.core.net.toUri
import androidx.media3.common.MediaItem
import androidx.media3.common.util.UnstableApi
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldBeEmpty
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldBeFalse
import org.amshove.kluent.shouldBeTrue
import org.junit.Test
@@ -7,6 +7,7 @@
package org.moire.ultrasonic.service
import android.net.Uri
import androidx.media3.common.C
import androidx.media3.common.PlaybackException
import androidx.media3.common.util.UnstableApi
@@ -15,7 +16,6 @@ import androidx.media3.datasource.HttpDataSource.HttpDataSourceException
import androidx.media3.exoplayer.source.LoadEventInfo
import androidx.media3.exoplayer.source.MediaLoadData
import androidx.media3.exoplayer.upstream.LoadErrorHandlingPolicy.LoadErrorInfo
import android.net.Uri
import java.io.FileNotFoundException
import java.io.IOException
import java.net.ConnectException