Enhance logging and task execution handling in Android#1
Conversation
… background coroutine
There was a problem hiding this comment.
Pull request overview
This PR updates the core multiplatform logger to queue log/appender operations on a single-threaded coroutine worker, adds lazy message overloads for log calls, and tweaks platform appenders and build toolchain settings.
Changes:
- Replaces mutex/runBlocking synchronization in
LOGwith a coroutine/Channel-backed serial work queue and adds lambda-based logging overloads. - Adjusts Apple/Android appenders’ formatting behavior (stack trace concatenation; chunk splitting output).
- Updates Gradle JVM toolchain from 8 to 11.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/commonMain/kotlin/com/mindera/lodge/LOG.kt |
Serializes add/remove/log operations via a coroutine worker; adds lazy-message overloads; deduplicates appenders by loggerId. |
src/appleMain/kotlin/com/mindera/lodge/appenders/OSAppender.kt |
Combines message + stack trace into a single OS log call. |
src/androidMain/kotlin/com/mindera/lodge/appenders/LogcatAppender.kt |
Simplifies chunk splitting logic for long messages. |
build.gradle.kts |
Moves compilation toolchain to JDK 11. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun add(appender: Appender) = tasks.trySend { | ||
| if (appenders.none { it.loggerId == appender.loggerId }) { | ||
| appenders.add(appender) | ||
| } else { | ||
| log("LOG", WARN, null) { "Appender '${appender.loggerId}' discarded: an appender with that id is already registered." } |
There was a problem hiding this comment.
add(appender: Appender) now returns the result of tasks.trySend(...) (a ChannelResult<Unit>), which changes the public API from returning Unit and makes it inconsistent with the other add/remove methods. Consider switching back to a block body returning Unit (and optionally handling a failed send explicitly) to avoid a breaking signature change for consumers.
| fun add(appender: Appender) = tasks.trySend { | |
| if (appenders.none { it.loggerId == appender.loggerId }) { | |
| appenders.add(appender) | |
| } else { | |
| log("LOG", WARN, null) { "Appender '${appender.loggerId}' discarded: an appender with that id is already registered." } | |
| fun add(appender: Appender) { | |
| tasks.trySend { | |
| if (appenders.none { it.loggerId == appender.loggerId }) { | |
| appenders.add(appender) | |
| } else { | |
| log("LOG", WARN, null) { "Appender '${appender.loggerId}' discarded: an appender with that id is already registered." } | |
| } |
| private val tasks = Channel<() -> Unit>(UNLIMITED) | ||
|
|
There was a problem hiding this comment.
Using Channel.Factory.UNLIMITED for the work queue means logging spikes can grow the queue without bound and potentially exhaust memory. For a logger, consider a bounded capacity with an explicit onBufferOverflow strategy (drop oldest/latest) or a non-blocking backpressure approach, and treat appender add/remove separately if they must be lossless.
| private var delayMillis = 0L | ||
|
|
||
| /** | ||
| * A dedicated coroutine that pulls lambdas out of `tasks` and executes | ||
| * them one-by-one, preserving the exact order in which they were queued. | ||
| */ | ||
| @Suppress("unused") | ||
| private val job = CoroutineScope(SupervisorJob() + Default.limitedParallelism(1)).apply { | ||
| launch(start = UNDISPATCHED) { | ||
| for (task in tasks) { | ||
| task() | ||
| delay(delayMillis) | ||
| } |
There was a problem hiding this comment.
delayMillis is only used in the worker loop but is never written anywhere in the codebase, so the delay(delayMillis) currently adds complexity without providing behavior. Either remove the delay entirely, or add an explicit (documented) API for configuring it so the field isn’t dead code.
| @Suppress("unused") | ||
| private val job = CoroutineScope(SupervisorJob() + Default.limitedParallelism(1)).apply { | ||
| launch(start = UNDISPATCHED) { | ||
| for (task in tasks) { | ||
| task() | ||
| delay(delayMillis) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The job property is not a Job (it’s a CoroutineScope returned from apply) and is only kept for side effects. Renaming it to reflect what it actually is (e.g., workerScope/worker) will reduce confusion for future maintainers.
| private fun log( | ||
| tag: String, | ||
| severity: SEVERITY, | ||
| t: Throwable?, | ||
| text: String | ||
| ) = mutex.withLock { | ||
| if(appenders.isNotEmpty()) { | ||
| val log = "[T#$threadName] | $text" | ||
| appenders.forEach { | ||
| if (it.minLogLevel.ordinal > severity.ordinal) return@forEach | ||
| it.log(severity, tag, t, log) | ||
| message: () -> String, | ||
| ) { | ||
| val originalThread = threadName | ||
| tasks.trySend { | ||
| if (appenders.isNotEmpty()) { | ||
| val log = "[T#$originalThread] | ${message()}" | ||
| appenders.forEach { | ||
| if (it.minLogLevel.ordinal > severity.ordinal) return@forEach | ||
| it.log(severity, tag, t, log) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
add/remove/log are now fire-and-forget enqueues (trySend) rather than completing synchronously as they did with the previous Mutex.withLock(runBlocking) approach. This is a behavior change for callers (e.g., add() returning before the appender is actually registered, logs potentially not flushed before process exit). Consider documenting this explicitly and/or providing a synchronous/flush option, and handle trySend failures so logs aren’t silently dropped if the channel is closed.
…severity initials
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private val job = CoroutineScope(SupervisorJob() + Default.limitedParallelism(1)).apply { | ||
| launch(start = UNDISPATCHED) { | ||
| for (task in tasks) { | ||
| task() |
| fun add(appender: Appender) = tasks.trySend { | ||
| if (appenders.none { it.loggerId == appender.loggerId }) { | ||
| appenders.add(appender) | ||
| } else { | ||
| log("LOG", WARN, null) { "Appender '${appender.loggerId}' discarded: an appender with that id is already registered." } | ||
| } | ||
| } |
| fun remove(id: String) { | ||
| tasks.trySend { appenders.removeAll { id == it.loggerId } } | ||
| } | ||
|
|
||
| /** | ||
| * Remove log appenders | ||
| * | ||
| * @param ids Log ids of each of the loggers enabled by the order sent | ||
| */ | ||
| fun remove(ids: Set<String>) = mutex.withLock { | ||
| this.appenders.removeAll { ids.contains(it.loggerId) } | ||
| fun remove(ids: Set<String>) { | ||
| tasks.trySend { appenders.removeAll { ids.contains(it.loggerId) } } | ||
| } |
| log("LOG", WARN, null) { "Appender '${candidate.loggerId}' discarded: an appender with that id is already registered." } | ||
| } | ||
| } | ||
| } |
| ) { | ||
| val originalThread = threadName | ||
| tasks.trySend { | ||
| if (appenders.isNotEmpty()) { | ||
| val log = "[T#$originalThread] | ${message()}" | ||
| appenders.forEach { | ||
| if (it.minLogLevel.ordinal > severity.ordinal) return@forEach | ||
| it.log(severity, tag, t, log) | ||
| } | ||
| } | ||
| } |
No description provided.