こんにちは。コミュニケーションアプリLINEのクライアントを開発している石川です。
この記事では、DroidKaigi 2023 の企業ブースで行った Code Review Challenge の 3 問目の解説をします。 Code Review Challengeについてはこちらを参照してください。
出題タイトル: Slip Through My Async
3 問目では、以下のコードが出題されました。
/**
* A repository class to save/load a [ShopItem] to the local storage.
*/
class ShopItemListRepository(
private val shopItemDao: ShopItemDao,
private val shopItemThumbnailDao: ShopItemThumbnailDao,
private val errorReporter: ErrorReporter
) {
/** [CoroutineScope] for DAO requests. This is to make jobs non-cancellable and running IO dispatcher. */
private val scope: CoroutineScope = CoroutineScope(NonCancellable + Dispatchers.IO)
/**
* Stores the given [ShopItem] to the local storage with a thumbnail. Then, returns true iff the item and thumbnail
* have been stored successfully.
*
* Even if the caller coroutine is cancelled, this continues to storing data to prevent a race condition between
* `ShopItem` and the thumbnail. This function is safe to call concurrently because of the non-cancellable coroutine
* scope.
*/
suspend fun store(shopItem: ShopItem): Boolean {
val thumbnail = createThumbnail(shopItem)
val differed = scope.async {
// Step 1: Store thumbnail.
// `suspendCoroutine` is to convert the callback interface of `shopItemThumbnailDao` to coroutine.
var thumbnailCallback: ((Boolean) -> Unit)? = null
val isThumbnailStoreSuccessful = suspendCoroutine { continuation ->
val callback = { isSuccessful: Boolean -> continuation.resume(isSuccessful) }
thumbnailCallback = callback
shopItemThumbnailDao.registerCallbackFor(shopItem, callback)
shopItemThumbnailDao.save(shopItem, thumbnail)
}
val callback = thumbnailCallback
if (callback != null) {
shopItemThumbnailDao.unregisterCallback(callback)
}
if (!isThumbnailStoreSuccessful) {
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store thumbnail")
return@async false
}
// Step 2: Store shop item.
shopItemDao.upsert(shopItem)
return@async true
}
return try {
differed.await()
} catch (exception: DataBaseQueryException) {
// At the step 2, this exception may be thrown.
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store an item", exception)
false
}
}
private suspend fun createThumbnail(shopItem: ShopItem): Bitmap {
/* snip */
}
}
// Old Java code
public class ShopItemThumbnailDao {
public void registerCallbackFor(@NonNull ShopItem item, @NonNull Consumer<Boolean> callback) { /* snip */ }
public void save(@NonNull ShopItem item, @NonNull Bitmap thumbnail) { /* snip */ }
public void unregisterCallback(@NonNull Consumer<Boolean> callback) { /* snip */ }
}この ShopItemListRepository は、ShopItem を保存/取得するためのクラスです。出題できるコード長の制約のため、設問のコードでは保存を行う store(ShopItem) のみ実装されています。
store(ShopItem) 関数では、大きく 2 つのことを行っています。
shopItemからサムネイルを生成し、shopItemThumbnailDaoに保存するshopItemをshopItemDaoに保存する
これら 2 つの保存の動作は、Kotlin coroutine の async 内で行っています。サムネイル保存後かつ ShopItem 保存前にキャンセルされた場合、 ShopItemDao と ShopItemThumbnailDao 間で、不整合な状態になってしまうため、それを防ぐために async を使っています。
また、サムネイルの保存を行う ShopItemThumbnailDao は、Java のコードとして実装されており、コールバックを使って保存の結果を通知するというインターフェースになっています。
問題点と解決策
このコードにある主な問題点として、以下の 4 つが挙げられます。
async内の例外の問題- 排他制御の問題
store関数の構造の問題- SAM 変換に関する問題
1: async 内の例外の問題
async 内で呼ばれている shopItemDao.upsert(shopItem) は、DataBaseQueryException という例外を投げる可能性がありますが、それは async 内でキャッチされておらず、await の呼び出しに対してキャッチを行っています。
return try {
differed.await()
} catch (exception: DataBaseQueryException) {
// At the step 2, this exception may be thrown.
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store an item", exception)
false
}この catch は、想定の通りに動きません。Android では、async 内で CancellationException 以外の例外が発生した時、CoroutineExceptionHandler が指定されない場合は、await の呼び出しでキャッチしてもアプリがクラッシュします。
これを解決するためには、async 内で例外をキャッチし、エラーを示す値を async の戻り値として返却するとよいでしょう。今回の場合は、false を返せば十分です。
val differed = scope.async {
// (省略)
try {
shopItemDao.upsert(shopItem)
true
} catch (exception: DataBaseQueryException) {
false
}
}他の回避方法としては、supervisorScope や CoroutineExceptionHandler を使うこともできます。ただし、回復可能なエラーに対しては、戻り値を使う方がミスが起きにくく、呼び出し元のコードも単純になります。特に強い理由がない場合は、coroutine 内の例外は、できるかぎり早い段階で型安全な戻り値に変換してしまうのが無難でしょう。
2: 排他制御の問題
store のドキュメンテーションでは、以下のように「並行して store を呼ぶとことができる」と書かれています。
/**
* (省略). This function is safe to call concurrently because of the non-cancellable coroutine
* scope.
*/しかし、実際の設問のコードは、ドキュメンテーションの仕様を満たしておらず、並行して store を呼び出すと競合状態が発生します。
競合状態の例 1: shopItemA と shopItemB という 2 つの ShopItem インスタンスがあり、これらの ID は同じで、他のプロパティは異なることを想定します。これらの ShopItem インスタンスから作られるサムネイル (thumbnailA と thumbnailB) が異なる場合、競合状態が発生し得ます。shopItemA と shopItemB の保存をほぼ同時に行った場合、以下の順序で保存が実行される可能性があります。
thumbnailAの保存thumbnailBの保存shopItemBの保存shopItemAの保存
この順番で保存が実行されると、shopItemA に対応するサムネイルが thumbnailB になってしまいます。
競合状態の例 2: 全く同じ shopItem に対して 2 回 store を呼び出すことを想定します (callA と callB とします)。ここで、callA のサムネイルの保存が失敗し、callB は成功したとしましょう。このとき、callA の continuation.resume の前に callB の registerCallbackFor が呼び出されると、callA と callB の両方で isThumbnailStoreSuccessful の値が false (= サムネイルの保存が失敗) になってしまいます。その結果、実際にはサムネイルが保存されているにも関わらず、ShopItem が保存されないという事態が発生しえます。
このような問題を解決するためには、Mutex などを使って排他制御を行う必要があります。もし、保存を並行で行う必要がない場合は、以下のように単一の Mutex インスタンスを使うだけでも十分でしょう。ただしその場合は、このレポジトリクラスで同時に使われるインスタンスは高々 1 つであり、また、このクラス以外で各 DAO のアクセスをしないという制約が必要になります。
private val mutex: Mutex = Mutex()
suspend fun store(shopItem: ShopItem): Boolean {
... = scope.async {
mutex.withLock {
// 保存のコード. 省略
}
}
...
}さらなる改善案として、以下のように async と withLock を 1 つの拡張関数としてまとめる方法もあります。
companion object {
private fun <T> CoroutineScope.asyncWithLock(mutex: Mutex, block: suspend CoroutineScope.() -> T): Deferred<T> =
async { mutex.withLock { block() } }
}3. store 関数の構造の問題
store が行っていることは、「サムネイルと ShopItem の 2 つを保存している」と端的に表現できます。しかし、store 関数のコードを読んでこれを理解するには、「サムネイルの保存はコールバックを使っている」といった背景知識も必要になります。そのため、store のコードを斜め読みをしても、何をする関数なのかがわかりにくいという問題があります。
この問題を解決するためには、コードの一部を補助的な関数として抽出し、斜め読みで関数の流れが理解できる構造にする必要があります。今回の場合は、「サムネイルの保存」と「ShopItem の保存」が重要であるため、その 2 点が強調されるような構造にすればよいでしょう。
suspend fun store(shopItem: ShopItem): Boolean {
val deferred = scope.asyncWithLock(mutex) {
val isThumbnailStoreSuccessful = storeThumbnail(shopItem)
if (!isThumbnailStoreSuccessful) {
// エラー処理
return@asyncWithLock false
}
val isShopItemStoreSuccessful = storeShopItem(shopItem)
if (!isShopItemStoreSuccessful) {
// エラー処理
return@asyncWithLock false
}
true
}
return deferred.await()
}
private suspend fun storeThumbnail(shopItem: ShopItem): Boolean { /* 省略 */ }
private suspend fun storeShopItem(shopItem: ShopItem): Boolean { /* 省略 */ }また、「store 内の async がキャンセルされない」という仕様は、あくまでも store 自身の都合であるため、補助的な関数 storeThumbnail を抽出した場合、その関数内では suspendCoroutine の代わりにキャンセル可能な suspendCancellableCoroutine を使うべきでしょう。
4. SAM 変換に関する問題
設問のコードでは、ShopItemThumbnailDao に使うコールバックは (Boolean) -> Unit として取り扱われています。しかし、ShopItemThumbnailDao の各メソッドの引数は Consumer<Boolean> と定義されています。この場合、メソッドを呼び出す際に、(Boolean) -> Unit が自動的に Consumer<Boolean> に変換されます。このような変換を SAM (Single Abstract Method) 変換と言います。
この SAM 変換によって、unregisterCallback が正しく動かないコードになっています。SAM 変換は、それが行われる度に異なるインスタンスを作ります。そのため ShopItemThumbnailDao.registerCallbackFor と .unregisterCallback の呼び出しで、異なる Consumer インスタンスが渡されます。結果として、一度登録したコールバックが二度と解除できないコードになってしまっています。
この問題を解決するためには、(Boolean) -> Unit の代わりに、明示的に Consumer<Boolean> のインスタンスを保持すればよいです。その際、誤ったリファクタリングを避けるためにも、以下のように Consumer を使っている理由をコメントで説明するのが望ましいでしょう。
/**
* Adapter of thumbnail storing callback of [ShopItemThumbnailDao] for coroutine continuation.
* This instance must be [Consumer] explicitly to ensure the same instance is used for callback registration and
* un-registration.
*/
private class ThumbnailStoreCallback(
private val continuation: Continuation<Boolean>,
private val dao: ShopItemThumbnailDao,
) : Consumer<Boolean> {
override fun accept(isSuccessful: Boolean) {
continuation.resume(isSuccessful)
dao.unregisterCallback(this)
}
}同様の問題は、SAM 変換に限らず、Java のオートボクシング等の自動的な変換一般に発生します。オブジェクトの同一性が重要になる場合、特に気をつける必要があります。
その他の問題
この他にも、設問のコードには大小様々な問題があります。
CoroutineScope の NonCancellable は不要:
異なる CoroutineScope で async を呼んだ場合、呼び出し元がキャンセルされても、実行は継続されます。この NonCancellable は削除しても問題ありません。
Dispatcher.IO の範囲が不要に大きい:
ShopItemThumbnailDao はコールバックを使った非同期処理であるため、IO で実行する必要はありせん。非同期コールバックを使うインターフェースの場合、呼び出し側が特定のスレッドを期待している可能性もあります。その場合は、IO が競合状態の原因になりえます。
ShopItemDao.upsert で Dispatchers.IO が必要な場合、upsert 内部で指定するべきです。仮に upsert 自身が IO を指定していない場合、その upsert についてだけ IO を指定すれば十分です。
これを改善するには、upsert だけを withContext(Dispatchers.IO) でラップするとよいでしょう。
thumbnailCallback が不要に大きなスコープに置かれている:
変数のスコープが不要に大きいことが、再代入可能で null チェックが必要なコードの原因になっています。これを解決するためには、suspendCoroutine 内でコールバックオブジェクトを作り、コールバック自身に unregisterCallback を呼ばせればよいでしょう。
upsert が失敗したときに、保存したサムネイルが残り続ける:
「サムネイルの保存は成功したが、ShopItem の保存に失敗した」ケースが考慮されていません。ShopItem の保存失敗時にサムネイルが残り続けないように、削除する必要があります。
(optional) async 末尾の return@async は不要:
末尾の式の評価結果は、そのままラムダの戻り値にできます。実際に return@async を消すかどうかは、チームのコーディング規約に従ってください。
deferred のスペルミス:
deferred とすべきところが differed になっています。実はこれは想定外のミスで、Code Review Challenge 中に指摘してもらいました。
修正後のコード
上記の問題を全て解決すると、以下のようなコードになります。
/**
* A repository class to save/load a [ShopItem] to the local storage.
*/
class ShopItemListRepository(
private val shopItemDao: ShopItemDao,
private val shopItemThumbnailDao: ShopItemThumbnailDao,
private val errorReporter: ErrorReporter
) {
/** [CoroutineScope] for DAO requests. This is to continue saving even after the caller coroutine is cancelled. */
private val scope: CoroutineScope = CoroutineScope(Dispatchers.Main)
private val mutex: Mutex = Mutex()
/**
* Stores the given [ShopItem] to the local storage with a thumbnail. Then, returns true iff the item and thumbnail
* have been stored successfully.
*
* Because the stores are different for [ShopItem] and the thumbnail, the storing is non-cancellable once the
* saving started.
*/
suspend fun store(shopItem: ShopItem): Boolean {
val deferred = scope.asyncWithLock(mutex) {
val isThumbnailStoreSuccessful = storeThumbnail(shopItem)
if (!isThumbnailStoreSuccessful) {
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store thumbnail")
return@asyncWithLock false
}
val isShopItemStoreSuccessful = storeShopItem(shopItem)
if (!isShopItemStoreSuccessful) {
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store item")
deleteThumbnail(shopItem)
return@asyncWithLock false
}
true
}
return deferred.await()
}
/**
* Saves thumbnail generated from the given [ShopItem]. Then returns true iff the storing is successful.
*/
private suspend fun storeThumbnail(shopItem: ShopItem): Boolean {
val thumbnail = createThumbnail(shopItem)
return suspendCancellableCoroutine { continuation ->
val callback = ThumbnailStoreCallback(continuation, shopItemThumbnailDao)
shopItemThumbnailDao.registerCallbackFor(shopItem, callback)
shopItemThumbnailDao.save(shopItem, thumbnail)
}
}
/**
* Saves the given [ShopItem]. Then returns true iff the storing is successful.
*/
private suspend fun storeShopItem(shopItem: ShopItem): Boolean =
withContext(Dispatchers.IO) {
try {
shopItemDao.upsert(shopItem)
true
} catch (exception: DataBaseQueryException) {
false
}
}
private suspend fun createThumbnail(shopItem: ShopItem): Bitmap {
/* snip */
}
private suspend fun deleteThumbnail(shopItem: ShopItem) {
/* snip */
}
/**
* Adapter of thumbnail storing callback of [ShopItemThumbnailDao] for coroutine continuation.
* This instance must be [Consumer] explicitly to ensure the same instance is used for callback registration and
* un-registration.
*/
private class ThumbnailStoreCallback(
private val continuation: Continuation<Boolean>,
private val dao: ShopItemThumbnailDao,
) : Consumer<Boolean> {
override fun accept(isSuccessful: Boolean) {
continuation.resume(isSuccessful)
dao.unregisterCallback(this)
}
}
companion object {
private fun <T> CoroutineScope.asyncWithLock(mutex: Mutex, block: suspend CoroutineScope.() -> T): Deferred<T> =
async { mutex.withLock { block() } }
}
}まとめ
この記事では、Code Review Challengeの 3 問目の解説として、以下の改善が必要なことを説明しました。
async内の例外を正しく取り扱う。- coroutine を使う場合は競合状態に注意する。
- 関数の流れを斜め読みで把握しやすいような構造にする。
- SAM 変換といった自動的な変換を使う場合、オブジェクトの同一性に注意する。
コードが複雑になってくると、その理解に過剰に集中してしまい、複合要因が絡むバグや可読性を下げている原因に目が向きにくくなります。その場合、まずは「理解がしにくい」こと自体を認識することで、原因を探り出すことができます。そうしてコードを整理した結果、複合要因が絡むバグも発見できる可能性も高まります。