Remove unnecessary Dispatcher.IO usage by romansl · Pull Request #4125 · meshtastic/Meshtastic-Android

romansl · 2026-01-03T08:42:58Z

This PR implements best practices for working with coroutines in the project. The main idea is to use withContext only to make a suspend function non-blocking. All calls that do not adhere to this principle have been removed.

https://developer.android.com/kotlin/coroutines/coroutines-best-practices#main-safe
https://elizarov.medium.com/blocking-threads-suspending-coroutines-d33e11bf4761

Copilot

Pull request overview

This PR removes unnecessary Dispatchers.IO usage from repositories, data sources, and ViewModels where operations are already asynchronous (Room DAOs, Ktor HTTP calls). However, the PR also makes several problematic changes by removing or changing dispatchers for operations that genuinely require specific thread pools.

Key Changes:

Removed withContext(Dispatchers.IO) wrapping from Room DAO suspend function calls across repositories and data sources
Removed flowOn(Dispatchers.IO) from flows backed by Room DAO queries
Changed CoroutineScope dispatchers from Dispatchers.IO to Dispatchers.Default in several Android services
Added justification comments for legitimate blocking I/O operations (file operations, ContentResolver)

Critical Issues Found:

Several Android services (RadioInterfaceService, MeshService, PacketHandler) had their scopes changed from Dispatchers.IO to Dispatchers.Default, which is inappropriate for services that perform I/O operations
ContactsViewModel now calls suspend functions synchronously within PagingData transformations, which will block the main thread
DatabaseManager's Room database building operation no longer runs on Dispatchers.IO, risking main thread blocking during database initialization

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
feature/settings/.../DebugViewModel.kt	Added CoroutineDispatchers injection, changed Dispatchers.IO to default, removed unnecessary IO wrapping
feature/settings/.../Debug.kt	Modifier chaining formatting improvements, added TODO comment for moving export logic to ViewModel
feature/settings/build.gradle.kts	Added core.di dependency for CoroutineDispatchers
feature/node/.../MetricsViewModel.kt	Removed dispatchers.io from viewModelScope.launch for DAO calls
feature/node/.../NodeActions.kt	Scoped Dispatchers.IO narrowly to AIDL service call only
feature/node/.../NodeDetailViewModel.kt	Similar scoped IO change for AIDL, removed from DAO calls
feature/node/.../PhoneLocationProvider.kt	Removed flowOn and CoroutineDispatchers dependency
feature/messaging/.../QuickChatViewModel.kt	Removed Dispatchers.IO from DAO operations, formatting improvements
feature/messaging/.../MessageViewModel.kt	Removed Dispatchers.IO from launches, parameter formatting improvements
feature/map/.../BaseMapViewModel.kt	Removed Dispatchers.IO from deleteWaypoint
feature/map/.../MapViewModel.kt	Added justification comments for legitimate file I/O operations
feature/firmware/.../FirmwareFileHandler.kt	Added justification comments for blocking file/network I/O
core/network/.../FirmwareReleaseRemoteDataSource.kt	Removed withContext wrapper from Ktor API call
core/network/.../DeviceHardwareRemoteDataSource.kt	Removed withContext wrapper from Ktor API call
core/database/.../DatabaseManager.kt	Removed withContext(IO) from buildRoomDb call (problematic)
core/data/.../TracerouteSnapshotRepository.kt	Used withTransaction, removed flowOn, changed mapLatest to map
core/data/.../QuickChatActionRepository.kt	Removed withContext and flowOn for DAO operations
core/data/.../PacketRepository.kt	Removed withContext wrappers from all DAO suspend calls
core/data/.../NodeRepository.kt	Removed withContext and flowOn from DAO operations
core/data/.../MeshLogRepository.kt	Removed flowOn, refactored telemetry parsing with dispatcher switching
core/data/.../DeviceHardwareRepository.kt	Removed withContext(IO) wrapper, restructured control flow
core/data/.../CustomTileProviderRepository.kt	Changed Dispatchers.IO to default for DataStore write (incorrect)
core/data/.../SwitchingNodeInfoWriteDataSource.kt	Removed withContext from DAO operations
core/data/.../FirmwareReleaseLocalDataSource.kt	Removed withContext, improved logic with maxByOrNull
core/data/.../DeviceHardwareLocalDataSource.kt	Removed withContext from DAO operations
app/.../ContactsViewModel.kt	Removed runBlocking from PagingData transformation (critical issue)
app/.../PacketHandler.kt	Changed scope from IO to Default (problematic for I/O service)
app/.../MeshService.kt	Changed scope from IO to Default (problematic for I/O service)
app/.../RadioInterfaceService.kt	Changed scopes from IO to Default (critical issue for radio I/O)
app/.../NetworkRepository.kt	Removed flowOn from callback flows

Comments suppressed due to low confidence (1)

core/data/src/google/kotlin/org/meshtastic/core/data/repository/CustomTileProviderRepository.kt:106

Changing from Dispatchers.IO to dispatchers.default for DataStore preferences write is incorrect. DataStore operations (mapTileProviderPrefs.customTileProviders = jsonString) are already main-safe and handle their own threading. The withContext wrapper is unnecessary entirely and should be removed, not changed to a different dispatcher. JSON serialization here is lightweight and doesn't require dispatcher switching.

    private suspend fun saveDataToPrefs(providers: List<CustomTileProviderConfig>) {
        withContext(dispatchers.default) {
            try {
                val jsonString = json.encodeToString(providers)
                mapTileProviderPrefs.customTileProviders = jsonString
            } catch (e: SerializationException) {
                Logger.e(e) { "Error serializing tile providers" }
            }
        }

Copilot · 2026-01-03T11:37:53Z

+                            unreadCount = packetRepository.getUnreadCount(contactKey),
+                            messageCount = packetRepository.getMessageCount(contactKey),


Calling suspend functions synchronously inside a Paging transformation is problematic. The packetRepository.getUnreadCount(contactKey) and packetRepository.getMessageCount(contactKey) calls are suspend functions being called within a map transformation on PagingData. This will execute on the main thread by default and block it.

The original code using runBlocking(Dispatchers.IO) was actually necessary here to avoid blocking the main thread, even though it's not ideal. A better solution would be to preload this data or restructure the data flow to avoid these synchronous calls inside the paging transformation.

Copilot · 2026-01-03T11:37:54Z

+        withContext(dispatchers.default) { // CPU-intensive operation, so run on default dispatcher
+            parseTelemetryLogActual(log)
+        }


The comment "CPU-intensive operation" is misleading. Protobuf parsing (Telemetry.parseFrom) and the builder operations are lightweight operations that don't require switching dispatchers. The original code didn't use any dispatcher switching for this function, and adding withContext(dispatchers.default) here introduces unnecessary dispatcher switching overhead. This should be a simple synchronous function call as it was originally.

Suggested change

withContext(dispatchers.default) { // CPU-intensive operation, so run on default dispatcher

parseTelemetryLogActual(log)

}

parseTelemetryLogActual(log)

Copilot · 2026-01-03T11:37:54Z


    private var queueJob: Job? = null
-    private var scope: CoroutineScope = CoroutineScope(Dispatchers.IO)
+    private var scope: CoroutineScope = CoroutineScope(Dispatchers.Default)


Changing the PacketHandler scope from Dispatchers.IO to Dispatchers.Default may be problematic. This scope is used for packet processing which likely includes database operations (via meshLogRepository, packetRepository) and potentially network I/O. While the PR description says to avoid wrapping Room DAO calls, this is a background service scope that coordinates I/O operations. Dispatchers.Default is optimized for CPU-bound work, not I/O operations that this handler performs. The original Dispatchers.IO was likely appropriate here.

Suggested change

private var scope: CoroutineScope = CoroutineScope(Dispatchers.Default)

private var scope: CoroutineScope = CoroutineScope(Dispatchers.IO)

Copilot · 2026-01-03T11:37:54Z


    private val serviceJob = Job()
-    private val serviceScope = CoroutineScope(Dispatchers.IO + serviceJob)
+    private val serviceScope = CoroutineScope(Dispatchers.Default + serviceJob)


Changing the MeshService scope from Dispatchers.IO to Dispatchers.Default is questionable. This is a long-running Android service that coordinates mesh network operations including database access, radio communication, and location services. Services that perform I/O operations (database, network, radio) should typically use Dispatchers.IO to avoid blocking the limited thread pool of Dispatchers.Default which is optimized for CPU-intensive work. This change could impact service performance and responsiveness.

Suggested change

private val serviceScope = CoroutineScope(Dispatchers.Default + serviceJob)

private val serviceScope = CoroutineScope(Dispatchers.IO + serviceJob)

Copilot · 2026-01-03T11:37:55Z


    /** We recreate this scope each time we stop an interface */
-    var serviceScope = CoroutineScope(Dispatchers.IO + SupervisorJob())
+    var serviceScope = CoroutineScope(Dispatchers.Default + SupervisorJob())


Changing RadioInterfaceService scope from Dispatchers.IO to Dispatchers.Default is problematic. This service manages radio communication interfaces (Bluetooth, TCP/IP) which are fundamentally I/O operations. The service reads and writes data to/from radio devices, which requires I/O dispatcher threads. Using Dispatchers.Default (optimized for CPU-bound work) for I/O operations can lead to thread pool exhaustion and poor performance.

Suggested change

var serviceScope = CoroutineScope(Dispatchers.Default + SupervisorJob())

var serviceScope = CoroutineScope(dispatchers.io + SupervisorJob())

Here, a dispatcher is needed only to have any dispatcher at all. But the default is more suitable here for two reasons: it consumes fewer resources and better expresses what we want to achieve, namely "use any dispatcher at all." If an IO dispatcher remained here, it might be misleading: as if we were using blocking operations somewhere. But that's not the case.

Copilot · 2026-01-03T11:37:55Z

-        val db =
-            dbCache[dbName]
-                ?: withContext(Dispatchers.IO) { buildRoomDb(app, dbName) }.also { dbCache[dbName] = it }
+        val db = dbCache[dbName] ?: buildRoomDb(app, dbName).also { dbCache[dbName] = it }


Removing withContext(Dispatchers.IO) from buildRoomDb call is problematic. Building/opening a Room database with RoomDatabase.Builder.build() performs blocking file I/O operations including opening SQLite database files, running migrations, and validating schemas. This should remain on Dispatchers.IO to avoid blocking the main thread or the calling coroutine's thread. The original code was correct.

The database builder is not blocking or IO call.

jamesarich · 2026-01-03T11:51:35Z

Hi @romansl , this PR seems pretty AI generated and wrong in most of the changes.

Please clean it up and provide a human generated reason why this is necessary.

romansl · 2026-01-03T16:34:20Z

Hi @romansl , this PR seems pretty AI generated and wrong in most of the changes.

Please clean it up and provide a human generated reason why this is necessary.

Yeah, AI wrote this comment based on my IDE and AI session. But it really captures the main goal of these changes - to bring in best practices for working with coroutines.

This PR (just like the last one) is kind of prep work - all about making the app more reliable and responsive. Next up, I’ll be focusing on best practices for error handling.

jamesarich · 2026-01-03T16:35:47Z

Unfortunately the AI is wrong. Give it another try.

jamesarich · 2026-01-03T16:44:10Z

Hi @romansl , this PR seems pretty AI generated and wrong in most of the changes.

Please clean it up and provide a human generated reason why this is necessary.

Yeah, AI wrote this comment based on my IDE and AI session. But it really captures the main goal of these changes - to bring in best practices for working with coroutines.

This PR (just like the last one) is kind of prep work - all about making the app more reliable and responsive. Next up, I’ll be focusing on best practices for error handling.

Before you work on much more, I would encourage you to reach out to the team on discord. We can get you further looped into our dev plans and process discussions.

Appreciate your help, just want to make sure we maintain good communication.

romansl · 2026-01-03T16:48:37Z

Unfortunately the AI is wrong. Give it another try.

Ok. Is this better?

jamesarich · 2026-01-03T16:53:05Z

Unfortunately the AI is wrong. Give it another try.

Ok. Is this better?

Unfortunately, no. I don't see any changes, just some justification for the AI. Also, the build is failing.

romansl · 2026-01-03T16:56:14Z

Unfortunately the AI is wrong. Give it another try.

Ok. Is this better?

Unfortunately, no. I don't see any changes, just some justification for the AI. Also, the build is failing.

I didn't really understand what's wrong. If the problem is in the code, then highlight the specific place with a comment.

jamesarich · 2026-01-03T16:58:15Z

Sorry, it's not on me to prove your code is bad, it's on you to prove your code is NOT bad.

romansl · 2026-01-03T17:00:44Z

The build is failing because Spotless is issuing a code formatting warning. But if I try to run spotlessApply locally, it modifies about 30 files in the project that I didn't even touch. And based on the warning, I can't figure out what it needs me to fix manually.

jamesarich · 2026-01-03T17:03:15Z

Especially when the PR seems highly AI generated - if the Author cannot explain the changes, why they are necessary, and their impact. This creates a low level of confidence in the submission, and we tend to be pretty leery of merging in suspect code that we then have to maintain.

romansl · 2026-01-03T17:05:32Z

Sorry, it's not on me to prove your code is bad, it's on you to prove your code is NOT bad.

The AI only modified 4 files (repositories), simply by removing withContext. All other changes I made manually.

jamesarich · 2026-01-03T17:11:43Z

Sorry, it's not on me to prove your code is bad, it's on you to prove your code is NOT bad.

The AI only modified 4 files (repositories), simply by removing withContext. All other changes I made manually.

Great, then you should be able to explain the changes and their necessity.

romansl · 2026-01-03T17:16:33Z

Great, then you should be able to explain the changes and their necessity.

The main idea is to use withContext only to make a suspend function non-blocking.

Please read the article by Roman Elizarov I mentioned.

These changes are necessary to reduce resource consumption and lower code complexity by removing unnecessary calls that are not beneficial and even harmful.

jamesarich

I’ve audited this locally and reviewed the documentation linked. This PR is unmerge-able as it currently introduces some dangerous architectural regressions.

While Room is main-safe for raw DAO hits, our repositories have to be main-safe for the entire operation . Stripping withContext(IO) indiscriminately pushes O(N) work—like the chunking in PacketRepository and the node filtering in NodeRepository—directly onto the UI thread. This will cause ANRs once a user has any real data.

The article linked actually reinforces the need for the repository to be main-safe so the caller doesn't have to worry about threading.

Keep the suspending getString changes in the ViewModel, but restore the IO dispatchers for any repository method doing more than a single DAO call.

romansl · 2026-01-06T06:19:32Z

tripping withContext(IO) indiscriminately pushes O(N) work—like the chunking in PacketRepository and the node filtering in NodeRepository—directly onto the UI thread. This will cause ANRs once a user has any real data.

None of the methods in PacketRepository require an IO dispatcher. Even deleteMessages, since a noticeable lag would only occur with a very large list, which is unlikely to form in this app due to limited memory. But if you still want to protect deleteMessages, you should use the Default dispatcher because the chunked function is CPU-bound, not IO. Also, the optimization can be done like this: uuidList.asSequence().chunked(500).

jamesarich · 2026-01-06T12:15:46Z

Closing this as no progress has been made towards resolving requested changes.

Instead of a large untested blanket arch change, maybe try again with small focused individual PRs, with tests and proof of optimization.

Fixed some unnecessary Dispetchers.IO calls.

5375007

jamesarich requested a review from Copilot January 3, 2026 11:33

Copilot started reviewing on behalf of jamesarich January 3, 2026 11:34 View session

Copilot AI reviewed Jan 3, 2026

View reviewed changes

jamesarich requested changes Jan 3, 2026

View reviewed changes

jamesarich closed this Jan 6, 2026

jamesarich mentioned this pull request Jan 8, 2026

refactor(deps): inject CoroutineDispatchers #4170

Merged

		unreadCount = packetRepository.getUnreadCount(contactKey),
		messageCount = packetRepository.getMessageCount(contactKey),

	private var scope: CoroutineScope = CoroutineScope(Dispatchers.Default)
	private var scope: CoroutineScope = CoroutineScope(Dispatchers.IO)

	private val serviceScope = CoroutineScope(Dispatchers.Default + serviceJob)
	private val serviceScope = CoroutineScope(Dispatchers.IO + serviceJob)

	var serviceScope = CoroutineScope(Dispatchers.Default + SupervisorJob())
	var serviceScope = CoroutineScope(dispatchers.io + SupervisorJob())

Uh oh!

Uh oh!

Conversation

romansl commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

romansl Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

romansl Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich commented Jan 3, 2026

Uh oh!

romansl commented Jan 3, 2026

Uh oh!

jamesarich left a comment

Choose a reason for hiding this comment

Uh oh!

romansl commented Jan 6, 2026

Uh oh!

jamesarich commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

romansl commented Jan 3, 2026 •

edited

Loading

jamesarich commented Jan 6, 2026 •

edited

Loading