こんにちは。コミュニケーションアプリ LINE のクライアントを開発している石川です。
この記事では、DroidKaigi 2022 の企業ブースで行った Code Review Challenge の 3 問目の解説をします。Code Review Challenge についてはこちらを参照してください。
出題タイトル: Truth in deep nests
3 問目では、以下のようなコードが出題されました。
class ProfileImageRepository(
private val context: Context,
private val metadataDao: ProfileImageMetadataDao = ProfileImageMetadataDao(),
private val apiClient: RemoteApiClient = RemoteApiClient(context),
private val ioScheduler: CoroutineContext = Dispatchers.IO
) {
private data class ProfileImageCache(
val userId: UserId,
val bitmap: Bitmap,
val expirationMillis: Long
)
private val fileCacheDirectory: File = context.cacheDir.resolve(PROFILE_IMAGE_DIRECTORY_NAME)
private val memoryCache: MutableMap<UserId, ProfileImageCache> = mutableMapOf()
suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) {
val memoryCachedData = memoryCache[userId]
?.takeIf { it.expirationMillis >= System.currentTimeMillis() }
if (memoryCachedData != null) {
return@withContext memoryCachedData.bitmap
}
val (bitmap, expirationMillis) = getProfileImageFromLocalCacheOrQuery(userId)
?: return@withContext null
memoryCache[userId] = ProfileImageCache(userId, bitmap, expirationMillis)
bitmap
}
private suspend fun getProfileImageFromLocalCacheOrQuery(
userId: UserId
): Pair<Bitmap, Long>? = withContext(ioScheduler) {
val cachedExpirationMillis =
metadataDao.queryExpirationMillis(userId) ?: Long.MIN_VALUE
val cacheFile = fileCacheDirectory.resolve(userId.stringValue)
val fileCachedBitmap = cacheFile.takeIf(File::exists)
?.readAsBitmap()
?.takeIf { cachedExpirationMillis < System.currentTimeMillis() }
if (fileCachedBitmap != null) {
return@withContext fileCachedBitmap to cachedExpirationMillis
}
val (bitmap, expirationMillis) = queryProfileImage(userId) ?: return@withContext null
metadataDao.storeExpirationMillis(userId, expirationMillis)
cacheFile.writeAsBitmap(bitmap)
bitmap to expirationMillis
}
private suspend fun queryProfileImage(userId: UserId): Pair<Bitmap, Long>? =
withContext(ioScheduler) {
val response = apiClient.queryProfileImage(userId.stringValue)
?: return@withContext null
val bitmap =
BitmapFactory.decodeByteArray(response.bitmapBytes, 0, response.bitmapBytes.size)
bitmap to response.expirationMillis
}
companion object {
private const val PROFILE_IMAGE_DIRECTORY_NAME = "profile_image"
}
}この `ProfileImageRepository` は `getProfileImage` という関数を持ち、それを呼び出すことで特定のユーザに対するプロフィール画像を取得できます。プロフィール画像は基本的に、`RemoteApiClient.queryProfileImage` によってネットワークを介して得られます。取得された画像は、メモリとローカルストレージそれぞれに期限付きのキャッシュとして保存されます。それらキャッシュは、次のように実装されています。
- オンメモリキャッシュ: `memoryCache: MutableMap<UserId, ProfileImageCache>` というプロパティに保存。ここで、`ProfileImageCache` はユーザID、画像、有効期限のタプル。
- ローカルストレージキャッシュ: 画像そのものを拡張関数 `File.writeAsBitmap` で保存。キャッシュの有効期限を `ProfileImageMetadataDao.storeExpirationMillis` で保存。
このクラスでは、有効期限内のキャッシュがあるならばそれを優先的に使い、そうでなければ `queryProfileImage` でネットワークアクセスを使うという手順でプロフィール画像を返します。
このコードには、大小様々な問題が潜んでいますが、主な問題は以下の3つです。
- 関数の呼び出し関係の問題
- `ProfileImageCache` や戻り値の構造の問題
- コルーチンの実行やキャンセルによる競合の問題
関数の呼び出し関係の問題
`getProfileImage` を呼び出したときは、「オンメモリキャッシュの確認 → ローカルストレージキャッシュの確認 → ネットワークを介したデータ取得」という流れで画像を取得します。しかし、`getProfileImage` の定義を見ただけでは、この流れを理解することは難しいです。その理由は `private` 関数の呼び出し関係にあります。この呼び出し関係を単純化すると、以下のようになります。
suspend fun getProfileImage(...): ... {
... // オンメモリキャッシュの確認
... = getProfileImageFromLocalCacheOrQuery(...)
... // オンメモリキャッシュの更新
return ...
}
private suspend fun getProfileImageFromLocalCacheOrQuery(...): ... {
... // ローカルストレージキャッシュの確認
... = queryProfileImage(...)
... // ローカルストレージキャッシュの更新
return ...
}
private suspend fun queryProfileImage(...): ... {
return ... // ネットワークを介した画像の取得
}`getProfileImage` ではオンメモリキャッシュの詳細が直接書かれ、そこから「ローカルストレージとネットワークアクセスに責任を持つ」という`getProfileImageFromLocalCacheOrQuery`を呼び出しています。また `getProfileImageFromLocalCacheOrQuery` でも、直接ローカルストレージキャッシュの詳細が書かれ、ネットワークアクセスについては`queryProfileImage`の呼び出しで行われます。
コードを読む人にとってはまず、「オンメモリ → ローカルストレージ → ネットワーク」という流れを理解しないと、動作の把握は難しいでしょう。しかし、それを知るためにはコードの詳細を読む必要がある点が、読みにくさの原因になっています。
これを解決するためには、オンメモリ、ローカルストレージ、ネットワークに責任をもつコードをそれぞれクラスや関数の単位で分離し、「`getProfileImage` ではそれらを順番に使うだけ」という構造にするべきです。抽象的なコードで説明すると、以下の `bad` 関数よりも `good` 関数の方が「何をする関数なのか」が把握しやすいということです。
fun bad() {
... /* doA */
doBAndC()
}
private fun doBAndC() {
... /* doB */
doC()
}
private fun doC() { ... /* doC */ }
fun good() {
doA()
doB()
doC()
}
private fun doA() { ... /* doA */ }
private fun doB() { ... /* doB */ }
private fun doC() { ... /* doC */ }今回は、キャッシュに対する操作を関数として分ける案を採用してみます。キャッシュの操作には、キャッシュの取得とキャッシュの更新があるため、それぞれに対応した `private` 関数を作りましょう。そして、分割された関数を `getProfileImage` から呼び出すように再構成することで、「`getProfileImage` が何をするか」が分かりやすくなります。
suspend fun getProfileImage(userId: UserId): Bitmap {
val profileImage = loadOnMemoryCache(...)
?: loadLocalStorageCache(...)
?: queryToServer(...)
storeLocalStorageCache(...)
storeOnMemoryCache(...)
return profileImage
}
private suspend fun loadOnMemoryCache(...): ... { ... }
private suspend fun loadLocalStorageCache(...): ... { ... }
private suspend fun queryToServer(...): ... { ... }
private suspend fun storeOnMemoryCache(...): {
if (...) return // キャッシュの更新が不要なら早期リターン
... // オンメモリキャッシュの追加・上書き
}
private suspend fun storeLocalStorageCache(...): {
if (...) return // キャッシュの更新が不要なら早期リターン
... // ローカルストレージキャッシュの追加・上書き
}「キャッシュの更新が本当に必要か」という判断も、`getProfileImage` 関数内ではなく、各 `store...Cache` 内で行うことで見通しの良いコードになります。このとき、`enum class SourceType { ON_MEMORY, LOCAL_STORAGE, SERVER }` という列挙型を定義すると、キャッシュの更新の要否の判断が簡単になるでしょう。
`ProfileImageCache` や戻り値の構造の問題
ここでは、以下の2つの問題を取り上げます
- `ProfileImageCache` の構造の問題
- 関数の戻り値 `Pair` が分かりにくいという問題
`ProfileImageCache` の問題
`ProfileImageCache` はオンメモリキャッシュ `memoryCache` のためだけに使われています。そのオンメモリキャッシュは `UserId` をキーとするマップとして定義されているのですが、その `UserId` は `ProfileImageCache` のプロパティと重複しています。そのような重複を許すと、例えば以下のような不正な状態を作る原因になります。
memoryCache[userId1] = ProfileImageCache(userId2, ...)オンメモリキャッシュが `UserId` をキーとするマップとして管理される以上、`ProfileImageCache` 自身は `UserId` を持っている必要はありません。以下のように `TimeLimitedImage` として定義し直すと良いでしょう。
class TimeLimitedImage(val bitmap: Bitmap, val expirationTimeInMillis: Long)ここで、`TimeLimitedImage` は `data class` ではなく、通常のクラスとして定義する方が好ましいでしょう。`copy` や `toString` を使う必要がないのも一つの理由ですが、等価性に関して誤解を招きかねないのが大きな理由です。`Bitmap` は独自の `equals` を定義していないため、`Bitmap.copy` を用いて `TimeLimitedImage` インスタンスの複製を作った場合、`==` での評価は `false` になります。これは、`Bitmap` の詳細を知っていないと勘違いしかねません。どうしても `data class` にしたいのであるならば、その注意点をクラスドキュメンテーションとして書く必要があります。
関数の戻り値の問題
`getProfileImageFromLocalCacheOrQuery` と `queryProfileImage` の戻り値の型は `Pair<Bitmap, Long>?` ですが、この型だけを見て戻り値の意味を予測するのは難しいでしょう。まだ、`Bitmap` の意味は関数の名前から予想しやすいですが、`Long` の意味を知るには、コードの詳細を読む必要があります。
このような場合は、関数のドキュメンテーションを書くことで戻り値の説明をするか、意味を付与できるようなデータ構造を使うと良いでしょう。たまたま先程 `TimeLimitedImage` を定義したので、それを流用することでこの問題を解決できます。
private suspend queryProfileImage(...): TimeLimitedImage? { ... }コルーチンの実行やキャンセルによる競合の問題
このコードでは、`Dispatchers.IO` のコンテキストで、以下のような操作を行っています。
- `memoryCache` の読み書き
- `storeExpirationMillis` と `writeAsBitmap` の呼び出し
`Dispatchers.IO` は複数のスレッドの中からディスパッチを行うため、同じ `UserId` の画像に関する処理を並行して行う可能性があります。ここで、`memoryCache` のインスタンスは `mutableMapOf()` で作られます。Kotlin 1.7 の時点では `mutableMapOf` は `LinkedHashMap` のインスタンスを返しますが、これはスレッドセーフではありません。また、`storeExpirationMillis` と `writeAsBitmap` の2つの呼び出しもアトミックではないため、有効期限と画像ファイルが競合する可能性があります。その他にも、`readAsBitmap` と `writeAsBitmap` をどう実装しているかにもよりますが、不正なファイルが見えたり、ファイルを破壊する可能性もあります。
この問題を解決するためには、`Mutex` を使うといった方法で、排他制御をする必要があります。また、単一の `Mutex` では粒度が大きすぎると思われる場合、代わりに `memoryCache` を `UserId` から `Deferred` へのマップとし、実際の画像の取得を別のコルーチンとして非同期に行う方法もあります。ちなみに、今回の場合は `sychronized` では正しく排他制御できません。詳しくは kotlinlang.org 公式のドキュメント https://kotlinlang.org/docs/shared-mutable-state-and-concurrency.html を参照してください。
また、`writeAsBitmap` は通常の関数として実装されているのか、それとも `suspend` として実装されているのかはこのコードを見ただけではわかりません。もし、`writeAsBitmap` が `suspend` 関数の場合、コルーチンのキャンセルに気をつける必要があります。`getProfileImage` を呼び出している `CoroutineScope` のキャンセルが `storeExpirationMillis` の直後に行われた場合、`writeAsBitmap` が実行されません。これにより「有効期限が設定されているのにも関わらず、画像ファイルが存在しない」という状況が発生します。この状況を正しく取り扱うことは難しいため、`storeExpirationMillis`・`writeAsBitmap` 間でキャンセルされないようにすると良いでしょう。その方法としては、例えば以下のようなものが存在します。
- `storeExpirationMillis` と `writeAsBitmap` を `withContext(NonCancellable)` で囲う。
- 外部からキャンセルされない新たな `CoroutineScope` を使って `launch` し、その中で `storeExpirationMillis` と `writeAsBitmap` を実行する。同期的な実行に見せかけたい場合は、`launch` の戻り値の `Job` に対して `join` を呼ぶ。
その他の問題
以上の 3 点が大きな問題でしたが、他にも細かい問題や、仕様に関する問題なども埋め込まれています。
- `getProfileImage` 内で直接行っている操作は、オンメモリキャッシュに対するものだけであるため、`withContext` は不要。
- `getProfileImage` という関数名は、短時間で実行されるという誤解を招きかねない。例えば `query...` といった名前に変えるべき。
- ローカルストレージキャッシュの有効期限の判定が逆。(誤: `<`, 正: `>=`)
- ローカルストレージキャッシュの有効期限の確認は `readAsBitmap` の前に行うべき。
- LRUキャッシュなどを用いて、キャッシュのサイズを制限するべき。
- `ioScheduler` には、通常 `Dispatchers.IO` が与えられるので、 `ioDispatcher` という名前のほうがより適切。
- etc...
まとめ
この記事では、Code Review Challenge の 3 問目の解説として、以下の改善が必要なことを説明しました。
- 関数の呼び出し関係を整理し、「何をしているのか」を明確にする
- プロパティの重複を避け、戻り値の意味を明示する
- 並行処理やキャンセルによる競合を修正する
なお、コルーチンの使い方に関しては、同チームの大石が Kotlin Fest 2022 にて「Androidアプリにおける Kotlin Coroutines のより良い使い方」https://fortee.jp/kotlin-fest-2022/proposal/930f6efb-c288-4497-9beb-1ba69f29cdbe というタイトルで発表する予定です。ぜひご視聴ください。
第4問目は安藤（https://engineering.linecorp.com/ja/blog/code_review_challenge_4）による解説です。