Hi everyone! Help me, please. How would you solve ...

# coroutines

[Sergey Shevtsov]

Hi everyone! Help me, please. How would you solve this issue?
I have a class which is a wrapper around the shared preferences. I added reactivity there with callbackFlow. It looks like this:

private val preferences = // shared preferences initialization

private val mutex = Mutex()

override fun observeString(prefKey: String, defaultValue: String?): Flow<String?> {
  return observeValueChanges(prefKey).mapWithLock {
    preferences.getString(prefKey, defaultValue)
  }.flowOn(<http://Dispatchers.IO|Dispatchers.IO>)
}

override fun observeBoolean(prefKey: String, defaultValue: Boolean): Flow<Boolean> {
  return observeValueChanges(prefKey).mapWithLock {
    preferences.getBoolean(prefKey, defaultValue)
  }.flowOn(<http://Dispatchers.IO|Dispatchers.IO>)
}

private fun observeValueChanges(prefKey: String): Flow<Unit> {
  return callbackFlow {
    val listener = OnSharedPreferenceChangeListener { _, key ->
      if (prefKey == key) {
        trySend(Unit)
      }
    }
    preferences.registerOnSharedPreferenceChangeListener(listener)
    send(Unit)
    awaitClose { preferences.unregisterOnSharedPreferenceChangeListener(listener) }
  }
}

private inline fun <T, R> Flow<T>.mapWithLock(crossinline transform: suspend (T) -> R): Flow<R> {
  return transform { value ->
    return@transform emit(
      mutex.withLock {
        transform(value)
      }
    )
  }
}

Coroutines mutex locking is used for synchronized write/read operations to avoid ConcurrentModificationException.
The issue is that registering/unregistering a listener is not thread safe. I can't just synchronize this with the coroutine mutex, because the awaitClose lambda is not suspend.
Possible solutions, in my opinion, are the following:
1. Creating a single thread dispatcher and calling registering/unregistering on them.
2. Just to use synchronized { } , but here's the question. How good is this practice of mixing concurrency of coroutines and no-coroutines?
I would be glad to see more elegant solutions, if any. Thank you.

[Nick Allen]

SharedPreferences appears to be thread safe: https://android.googlesource.com/platform/frameworks/base.git/+/master/core/java/android/app/SharedPreferencesImpl.java#256
Why do you think it'll throw a ConcurrentModificationException exception?

synchronized { ... } is fine to call with coroutines so long as you don't include suspend methods in the synchronized block, in which case, it has all the same benefits and risks as when used with non-coroutine code.

[denis090712]

ConcurrentModificationException is not about coroutines or thread-safe access.

It means that the collection you are currently iterating has changed.

Just try to copy your original collection and then use it.

Anyway, without a full stack trace, I can’t say what collection it is and whether there is a problem with SharedPreferences implementation.

[Sergey Shevtsov]

Maybe I didn't express myself clearly)

The issue with the ConcurrentModificationException has been solved with the coroutines mutex. I was getting this exception because I was modifying the pref file and reading it at the same time.

I asked about registering/unregistering of listeners. Without any kind of synchronization, these operations may be performed in the wrong order. I'm looking for a good way to synchronize them.

[Sergey Shevtsov]

In any case, thanks for your answers)
I'm more inclined to use the synchronized { ... } option.

[denis090712]

I wouldn’t recommend you use synchronized or any other java synchronization primitives with coroutines if you don’t fully understand their impact. If you don’t want to run into a deadlock or get other unexpected behaviour use thread confinement instead to achieve the same result.

[Nick Allen]

The issue with the ConcurrentModificationException has been solved with the coroutines mutex.

That's seems unlikely given that those methods are already thread safe. The entire bodies of getString and getBoolean are wrapped in synchronized blocks when I peek at the AOSP.

I asked about registering/unregistering of listeners. Without any kind of synchronization, these operations may be performed in the wrong order. I'm looking for a good way to synchronize them.

Same. Also, code above awaitClose will run entirely before code inside the awaitClose block.

[uli]

Are you using SharedPreferences? Or EncryptedSharedPreferences?

[Sergey Shevtsov]

Forgot to mention) But it's important. My bad.
I'm using EncryptedSharedPreferences. They are not thread safe like SharedPreferences.

[uli]

Yes, that’s evil. A drop in replacement that breaks the contract.

[uli]

Not only is it not thread safe, also the listeners are not called in the main thread as documented here, but within the call stack and thread of the editor.commit()/.apply() call!

[uli]

And almost no KDoc at all which makes you look at the SharedPreferences KDoc

[uli]

Maybe you can use another flow like

preferences.registerOnSharedPreferenceChangeListener(listener)
    send(Unit)
    var registered = MutableStateFlow<Boolean>(true)
    launch {
        registered.filter{ it.not }
        .first {
            preferences.unregisterOnSharedPreferenceChangeListener(listener)
        }
    }
    awaitClose { registered = false }

^^^Take that just as a sketch of the idea. This code has not seen an ide 🙂

Nah, then you could also just launch form awaitClose, both will probably give you other races

[uli]

another raw idea:
• register a single shared flow on the preferences that starts on first collection // This will avoid the concurrent modification of the listeners List
• then instead of registering a furhter listener, you just start collecting the shared flow