Search

LINE株式会社は、2023年10月1日にLINEヤフー株式会社になりました。LINEヤフー株式会社の新しいブログはこちらです。 LINEヤフー Tech Blog

Blog


Code Review Challenge 5問目解説

Hidetsugu Tamaki / Ralph2022-12-09

LINEのAndroidアプリを作っています。

こんにちは。コミュニケーションアプリ LINE のクライアントを開発している玉木です。

この記事では、DroidKaigi 2022 の企業ブースで行った Code Review Challenge の 5 問目の解説をします。Code Review Challenge についてはこちらを参照してください。

出題タイトル: Coroutine's hard-to-understand trap that is easy to use for us

5 問目では、以下のようなコードが出題されました。

class MyContentProvider : ContentProvider() {
  
    private val singleThreadCoroutineDispatcher: CoroutineDispatcher =
        Executors.newSingleThreadExecutor().asCoroutineDispatcher()
  
    // Do not request again if succeeded once.
    private val getResultCache: MutableMap<Long, String> = mutableMapOf()
      
    private val apiClient: ApiClient = ApiClient()
  
    override fun query(
        uri: Uri,
        projection: Array<out String>?,
        selection: String?,
        selectionArgs: Array<out String>?,
        sortOrder: String?
    ): Cursor {
        val id: Long = TODO() // Get id value from `uri`.
        val result = runBlocking {
            get(id)
        }
        return MatrixCursor(TODO()) // Create MatrixCursor from `result`.
    }
  
    private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) {
        val resultFromCache = getResultCache[id]
        if (resultFromCache != null) {
            return@withContext resultFromCache
        }
        val result = try {
            apiClient.get(id)
        } catch (_: Throwable) {
            showErrorMessage()
            null
        }
        getResultCache[id] = result
        return@withContext result
    }
  
    private suspend fun showErrorMessage() = withContext(Dispatchers.Main) {
        Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show()
    }
  
    private class ApiClient {
  
        @Throws(IOException::class)
        suspend fun get(id: Long): String = withContext(Dispatchers.IO) {
            // Access to the server with HTTP request.
            delay(100L)
            return TODO()
        }
    }
}

この `MyContentProvider` は、このコンテンツプロバイダに対してクエリが実行された際に何かしらのHTTPリクエストを実行してその結果をクエリ結果として返すようなコンテンツプロバイダです。

コンテンツプロバイダ は、アプリが保存しているデータへアプリ内外からアクセスする方法を提供するための、Android Framework が提供している手法です。抽象化レイヤーとして機能するだけではなく、プロセスをまたいだデータ通信を安全に行えたり、セキュリティ面でのサポートがあったりと様々なメリットがあります。

さらに追加的な機能として、HTTPリクエストの結果を正常に取得出来た場合はそのデータをキャッシュし、同じidのコンテンツへのクエリが再度あった場合はキャッシュされたデータを使用します。また、HTTPリクエストに失敗した場合はToastを用いてエラーメッセージを表示するような実装になっています。

このコードでは、以下の2つの大きな問題を持っています。

  • コルーチンとシングルスレッドを用いた排他制御の問題
  • 呼び出し元スレッドとrunBlockingを用いたコルーチンの起動の問題

コルーチンとシングルスレッドを用いた排他制御の問題

`get` 関数内では、「`getResultCache` にキャッシュされた結果があるかを確認し、あればそのデータをreturn、無ければ `apiClient` から結果を取得してreturn」という動作をします。この際、`get`関数全体が `Executors.newSingleThreadExecutor` から生成された `singleThreadCoroutineDispatcher` 上で実行されますが、まずはこのロジックの意味を考えてみたいと思います。

何のためにシングルスレッドを使うかと考えたとき、よくある理由としては実行順序の保証や、読み書きする変数のメモリ可視性やロジック自体の要件における排他制御などで使用されるケースが多いと思われます。

今回のケースではまさしく後者の用途として使用しています。`getResultCache` にキャッシュされた結果が無く、`apiClient` へ問い合わせている間に同じ id への問い合わせが起きることで2重にリクエストが行われてしまうことを防ぐために、`getResultCache` への問い合わせと結果の挿入が行われるまでの間で排他制御を行っています。

ただし、今回のコードではどのような意図を持って `singleThreadCoroutineDispatcher` が使われているのかを推測するのは、コードを簡単に読んだだけでは難しいかもしれません。レビュープロセスにおいて、もしこのような用途がわかりにくい何かを見つけた場合には、そのコードを書いた人になぜ必要なのかを質問し、必要に応じてクラスコメントやインラインコメントなどを追加してもらうというのが重要になります。当日のブースにおいても、何のためにこの `singleThreadCoroutineDispatcher` が必要なのかを聞かれた場合には排他制御のためと回答をしていました。

それでは `singleThreadCoroutineDispatcher` が `getResultCache` の排他制御のためだとわかったところで、コルーチンにおけるシングルスレッドを用いた排他制御の問題点について紹介をしたいと思います。

val singleThreadCoroutineDispatcher: CoroutineDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher()
 
suspend fun doOuter() = withContext(singleThreadCoroutineDispatcher) {
    println("start: outer")
    doInner()
    println("end: outer")
}
suspend fun doInner() = withContext(Dispatchers.IO) {
    println("start: inner")
    delay(1000L)
    println("end: inner")
}

上記のコードは問題文のコードをより簡略化したコードになります。`doOuter` 関数が `singleThreadCoroutineDispatcher` で実行されるような suspend 関数として定義されており、内部で別の dispatcher で実行される `doInner` 関数を呼んでいます。`doOuter` 関数を呼ぶと、以下のような出力が得られます。

start: outer
start: inner
// 1000ミリ秒
end: inner
end: outer

それでは、この `doOuter` 関数を 500ミリ秒ずらして2回呼んだ時はどのような出力になるでしょうか？ `doOuter` 関数はシングルスレッドで動作する `singleThreadCoroutineDispatcher` で実行されるような suspend 関数なので、1回目の `doOuter` の呼び出しが終わるまで、2回目の `doOuter` の呼び出しは開始しないように思われますが、実際は以下のような出力になります。

start: outer // 1回目の呼び出し
start: inner // 1回目の呼び出し
// 500ミリ秒
start: outer // 2回目の呼び出し
start: inner // 2回目の呼び出し 
// 500ミリ秒
end: inner // 1回目の呼び出し
end: outer // 1回目の呼び出し
// 500ミリ秒
end: inner // 2回目の呼び出し 
end: outer // 2回目の呼び出し

1回目の `doOuter` がまだ完了していない(`end: outer`が出力されていない) タイミングで2回目の `doOuter` の実行が始まり、2回目の `start: outer` が出力されています。

これは、「別の suspend 関数を起動した時、呼び出し元のコルーチンは"中断"状態になる」 というKotlin Coroutines の基本的な動作になります。このコードでは、1回目の `doOuter` の呼び出しが `doInner` を呼び出したときに `doOuter` 側のコルーチンが中断状態になります。中断状態になるとディスパッチャーなどのリソースは開放され、別のコルーチンがそのリソースを使用して起動しようとしている場合にはそちらの起動のために使われることになります。つまり、「`singleThreadCoroutineDispatcher` によってシングルスレッドで動作するようにしていても、内側で別のコルーチンが起動している場合にはそのスレッドで別のコルーチンが起動する可能性がある」ということになります。

これを踏まえて問題文のコードを見てみましょう。

private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) {
    val resultFromCache = getResultCache[id]
    if (resultFromCache != null) {
        return@withContext resultFromCache
    }
    val result = try {
        apiClient.get(id)
    } catch (_: Throwable) {
        showErrorMessage()
        null
    }
    getResultCache[id] = result
    return@withContext result
}

`get` 関数は `singleThreadCoroutineDispatcher` 上で実行されますが、内側で `apiClient.get` という別のコルーチンを起動しているため、 ` apiClient.get` の実行中は `singleThreadCoroutineDispatcher` を使用した別のコルーチン、つまりは別の `get` 関数が実行できてしまうことになります。そうすると、`getResultCache` に対する排他制御がきちんと行われていないことになり、同じidへの複数回のリクエストが発生してしまうことになります。

解決策

コルーチンを用いて排他制御を行う場合は `Mutex` を使うようにしましょう。

private val getResultCacheMutex: Mutex = Mutex()
 
private suspend fun get(id: Long): String = getResultCacheMutex.withLock {
    // snip
}

このようにすることで、`withLock` の内部のラムダの実行に排他制御をかけることが出来ます。また、`Mutex` 自身の名前にも「何のためのMutexなのか」といった情報を付加したり、必要に応じてインラインコメントを書くことで、なぜ排他制御が必要なのかという点についても他の開発者に伝わりやすくなります。

呼び出し元スレッドとrunBlockingを用いたコルーチンの起動の問題

次に、`query` 関数で用いている `runBlocking` 関数に関連する問題です。

`runBlocking` 関数はコルーチンを起動するための関数の一つで、呼び出しのスレッドをブロックしながらコルーチンを起動し、結果を同期的に取得することができます。`query` 関数は `ContentProvider` で定義されている関数であり Android Framework 側で指定されている関数であるため、この関数自体を suspend 関数にすることや、返り値以外の方法で結果を返すように変更することはできません。そのため、 `runBlocking` 関数を使用してコルーチンを起動しつつ、その結果を同期的に取得する必要があります。

ここまでは問題無さそうですが、`query` 関数がどのスレッドから呼ばれる可能性があるかどうかを考えてみると問題点が見えてきます。

この `query` 関数は直接私たちが呼ぶのではなく、 `ContentResolver` に対して行うクエリを経由して、Android Framework 側が呼ぶような仕組みになっています。そのため、`ContentProvider` クラスの JavaDoc やリファレンスをきちんと読む必要があります。

`ContentProvider` クラスの JavaDoc には以下のような記述があります。

> This method can be called from multiple threads, as described in Processes and Threads.

また、「Processes and Threads」のページにも以下のような記述があります。

> バインドされたサービスのメソッドなど、リモートで呼び出されるメソッドなどがこれに該当します。IBinder に実装されたメソッドに対する呼び出しが、IBinder が実行されているプロセスと同じプロセスで発生した場合、メソッドは呼び出し側のスレッドで実行されます。ただし、呼び出しが別のプロセスで発生した場合は、システムが IBinder と同じプロセスに保持するスレッドのプールから選ばれたスレッドでメソッドが実行されます（プロセスの UI スレッドでは実行されません）。

>同様に、コンテンツ プロバイダは、他のプロセスで発生したデータ リクエストを受け取ることができます。ContentResolver クラスと ContentProvider クラスによってプロセス間通信がどのように管理されているかが見えなくなりますが、それらのリクエストに応答する ContentProvider メソッド（query()、insert()、delete()、update()、getType()）は、プロセスの UI スレッドではなく、コンテンツ プロバイダのプロセスにあるスレッドのプールから呼び出されます。これらのメソッドは同時に複数のスレッドから呼び出される可能性があるため、先ほどと同様にスレッドセーフになるように実装する必要があります。

こちらのページではスレッドセーフに関して重点を置いた解説を行っているため、どのスレッドで実行されるかという点においては付加情報のように書かれていますが、`query` 関数の実行スレッドに関しては `IBinder` と同等のルールが適用されます。

つまり、「`ContentResolver` へ同一プロセスからアクセスしたときにはその呼び出し元スレッドと同一」「別プロセス(同一アプリの別プロセスおよび別アプリ)からのアクセスの場合はスレッドプール内のスレッド」ということになります。実際に `ContentProvider` を自作し、どのスレッドで実行されるかを確認するとこのような挙動になっているのが確認できます。

ここまでの話をまとめると、`query` 関数は 「同一プロセスからクエリされる場合はその呼び出し元のスレッドと同一」のスレッドから呼び出されることがわかりました。つまりはほぼ呼び出し元のスレッドの制約がないということであり、もちろん呼び出し元のスレッドとしてメインスレッドから呼び出される可能性もあります。

ここで問題になるのが、`apiClient.get` 関数の呼び出しでがエラーが起きたときに呼び出している `showErrorMessage` 関数です。

private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) {
    // snip
    val result = try {
        apiClient.get(id)
    } catch (_: Throwable) {
        showErrorMessage()
        null
    }
    // snip
}
 
private suspend fun showErrorMessage() = withContext(Dispatchers.Main) {
    Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show()
}

`showErrorMessage` 関数はトーストを表示する機能を持つため、 `Dispatchers.Main` によってメインスレッド上で実行するようなコルーチンとして定義されています。もし `showErrorMessage` 関数が呼ばれた場合、メインスレッド上で実行が完了するまで呼び出し元のコルーチンは中断されます。

一方、`query` 関数は同一プロセスからクエリされる場合はメインスレッドからも呼び出される可能性があり、メインスレッド上で `runBlocking` を呼ぶ可能性があります。

この2つの事象が同時に起きてしまった場合、「`runBlocking` でメインスレッドがブロッキングされている」のに「メインスレッドでしか起動できないコルーチンが起動」しようとしてしまい、メインスレッドに対してデッドロック状態に陥ってしまいます。

解決策

修正方法に関しては色々方法がありますが、コードによっては出来ない場合もあるので注意が必要です。

まず考えられるのは「`runBlocking` を安易に使用しない」というものです。なるべく避けられるのであれば使用を避け、Android Framework やライブラリなどとのつなぎ込みの箇所など、どうしても必要な箇所のみで使うようにします。使う場合は、実行元のスレッドを止めてしまうという性質があるため、内部で起動されるコルーチンがどのようなコルーチンかという点や、どのようなスレッドを止めてしまうのかという点をきちんと把握して使う必要があります。

今回の問題文のコードでは `runBlocking` を使わざるを得ない状況のため、次に考えられる修正方法としては「トーストの表示をしない(`withContext(Dispatchers.Main)` をしない)」というものがあります。特にメインスレッドを要求するような処理を `ContentResolver`のようなデータ処理のみを行うはずのクラス内で必要とするのは違和感があります。エラーの処理としては `ContentResolver` で推奨されている方法でエラーを呼び出し元へ返し、呼び出し元で適切にハンドリングして必要に応じて UI などへ表示するのが良いでしょう。

その他の問題

他にも問題文のコードには以下のような問題を埋め込んでいます。

  • クラス名: `MyContentProvider` だと何の `ContentProvider` かわからないので、より具体的な名前にする

  • クラス設計:

    • キャッシュ機能を入れる場合は、リポジトリクラスを作成するなどして `ContentProvider` の実装には直接書かないようにする。

    • テスタブルな設計にするために、`ApiClient` クラスも内部に書くのではなく、publicまたはinternalなクラスとして記述することでDI可能にする。

    • `Dispatcher` など、テスタブルな実装のために公式でインジェクトすることを推奨されているオブジェクトに関してはDI可能にする。

  • 命名: `get` だけでは何を取得するのかわかりにくいので `getHoge` といった命名にする。

  • 命名: `getResultCache` だと関数名のように見えてしまうので、名詞句になるように `fetchedHogeCache` などと命名する。そうするとネットワークから取得したことがわかりどういった内容が含まれるのかがわかりやすくなる。

  • try-catch: catchする例外はできる限り狭くする。(今回であれば `IOException` のみをcatchするようにする)。ThrowableをcatchするとError系の回復不可能な例外もcatchされてしまい、アプリが不安定な動作になる。

  • コメント: 「Do not request again if succeeded once.」 という「使い方」のコメントだけではどのような要素がこのマップに入るのかがわからないので、「目的」を明示する。 

  • etc...

また、以下のバグは私も気づかずに挿入していたガチのバグでした。

  • nullability: `get` 関数の返り値の型など、一部 `String?`にしなければいけない箇所がある。
  • miss: `Toast.makeText` の第1引数は `Context` が必要であるため、`this` ではなく `getContext()` にしなければいけない。

まとめ

この記事では、Code Review Challenge の 5 問目の解説として、以下の改善が必要なことを説明しました。

  • コルーチンを使用した際の排他制御を正しく行う
  • `runBlocking` を使用してコルーチンを起動する場合は、呼び出し元スレッドや呼び出すコルーチンに気をつける

これにて Code Review Challenge の解説シリーズは以上となります。

全ての問題解説は 「【DroidKaigi 2022】Code Review Challengeの概要と解説まとめ」 にリンクを掲載していますので、気になる問題をぜひ解いてみてください。

実際に DroidKaigi 2022 のブースでコードレビューにチャレンジしていただいた方にはお礼申し上げます。