go-sql-driver/mysql の QueryContext でコンテキストをキャンセルしたら race が起こる

タイトルの通り、 QueryContext の第一引数に渡した Context を、 Rows.Close() を呼び出す前にキャンセルすると、 race が起こる可能性があります。

修正する pull request を作成したのですが、メンテナが作ったプルリクエストは他のメンテナのレビューなしにマージしてはいけないというルールがあるのでまだマージしていません。なので、 QueryContext を利用するときは注意してください。

github.com

問題になるコード

Rows.Scan() の引数の型が *sql.RawBytes (sql.RawBytes の定義は type []byte RawBytes) だった場合、受け取った []byte の中身が他の goroutine から書き換えられることがあります。

rows, err := db.QueryContext(ctx, "select ...", ...)
if err != nil {
    // ...
}
defer rows.Close()

for rows.Next() {
    var sql.RawBytes
    err := rows.Scan(&buf)
    // どこかのタイミングで ctx がキャンセルされると、
    fmt.Println(buf) // buf の中身が他の goroutine から書き換えられる
}

背景

database/sql の設計は、 driver の API は1つの接続だけを扱う（コネクションプール部分は全て database/sql が解決する）、そしてその1つの接続に対して並行で driver の API が呼ばれることはない、ようになっています。

また、 rows.Scan() が sql.RawBytes を出力する場合、その参照先の部分は次の rows.Next() か rows.Close() 以降はドライバが再利用できるようになっています。これは受信バッファの中身を直接スライスで返すことで余分なアロケートとコピーを減らすためです。

コンテキスト対応が入るまではこれでうまくいっていました。しかし、 database/sql がコンテキストに対応する時に、コンテキストのキャンセルを別 goroutine で監視して、アプリケーションからの rows.Close() や rows.Scan() を待たずにドライバー側の rows.Close() を呼び出すようにしてしまったのです。

Go 1.10 の changelog

結果として次のような状況になりました。

• rows.Scan() が返した sql.RawBytes (type []byte RawByte)は受信バッファの中を直接参照しています。アプリケーションは次の rows.Next() か rows.Close() を呼び出すまではその中身を合法的に読むことができます。
• ドライバは rows.Close() や rows.Next() が呼ばれた場合、前回の Scan() で返した []byte の中身を破壊していいので、送受信バッファとして再利用します。
• database/sql はコンテキストがキャンセルされると別 goroutine からドライバ側の rows.Close() を呼び出します。

正直、なんてことしてくれたんだ！って感じです。アプリケーションが rows.Next() を呼ぶまで待ってくれたらいいのに。なんでわざわざ別 goroutine で。。。

たぶん一刻も早くキャンセルしたかったんでしょうが、MySQLでは百害あって一理ありません。 MySQL でクエリのキャンセルは難しいのでまだ実装してないし、たとえ実装していたとしても、 rows.Next() が一回でも呼ばれているということはクエリの実行はすでに終わっています。 rows.Next() を待たずにキャンセルするメリットが現在も将来も一切ありません。

2019-04-03 追記: sql.RawBytes を使わなくても race が起こる

ユーザーコードに直接受信バッファの中身を参照する []byte が渡されるのは sql.RawBytes を利用したときだけです。

しかし、 database/sql の sql.Rows.Scan() は、 driver.Rows.Next() が返した値と Scan の引数の間の変換処理をしています (convertrows)。この変換処理は driver.Rows.Next() が返した受信バッファ内を直接参照する []byte から string を作ったり、新たな []byte を作ってそこにコピーしたりしています。

タイミング悪くこの変換処理中にコンテキストがキャンセルされた (厳密には database/sql 内の監視 goroutine がコンテキストのキャンセルを検出して driver.Rows.Close() を呼び出した) 場合は、ユーザーコードで sql.RawBytes を使っていなくても race が発生します。

Sony の SBH90C レビュー

https://www.amazon.co.jp/dp/B07CZQMYKK/ref=as_li_ss_tl?&hvadid=274798673996&hvpos=1o3&hvnetw=g&hvrand=13734372561970267267&hvpone=&hvptwo=&hvqmt=&hvdev=c&hvdvcmdl=&hvlocint=&hvlocphy=1028853&hvtargid=pla-465703230864&th=1&psc=1&linkCode=ll1&tag=py0d-22&linkId=9446964464023e4d4ba0cec75ac27735&language=ja_JP

それまでは Anker の Zolo Liberty を使っていたけれども、遅延が激しくてアニメ鑑賞でも気になっていたので買い替えた。 Zolo Liberty は codec が AAC なうえに、完全ワイヤレスだから左右間の再転送も必要で遅くなっているんだと思う。2019年のTWS Plus と apt-x Adaptive に期待している。

SBH90C は apt-x に対応しているのと、ネックバンド型だから左右転送の遅延もなく、きっと無線も安定しているだろうというのでチョイス。 Type-C ケーブルでデジタル接続に対応しているのもオタク心を刺激する。

感想＆気になった点。

• 専用のType-C ケーブルの太さはまだ良いが、しなやかさがイヤホンケーブルとしてはちょっと。。。ケーブルを柔らかくできないなら有線はアナログのほうが良かったと思う。
• 有線時の遅延はスクフェスプレイに問題が無いレベル。
• イヤホンジャック利用時はスマホの横画面でどちらを上にしたほうがスクフェスプレイがやりやすいかがデフォルトの横画面の向きと逆で、「回転」をOn/Offする必要があった。 Type-C ジャックは中央にあるので画面表示に合わせてスマホを持てて楽。
• 無線の安定性は驚くほど上がった。中目黒駅での乗り換えで Liberty は壊滅状態だったが、 SBH90C は無問題。
• 無線の遅延も動画視聴してる限りは気にならない。（音ゲーは最初から期待してないので試してないが、それ以外のゲームなら多分問題ない）
• カナル型なのに、遮音性が驚くほど低い。「ポート」と呼ばれる穴があるせい。ノイズキャンセリングも無いので、通勤用途には厳しかった。セロテープかなにかでポートを塞いで見るつもり。
• torne mobile が USB 接続だとエラーになる！スクフェス後にそのままアニメ見たいときや、充電したいときに困る。大人の事情だとは思うけどこういうつまらない制限でユーザビリティを下げるのはやめてほしい。

結論: 通勤用途には WI-1000X (と Type-C → アナログ変換ケーブル) のほうが向いてそう。

MySQL Connector/C の現状

MySQL 8 が GA になってたときは MySQL がどういうつもりなのかいまいち分からなかったのですが、 8.0.13 リリースを機に改めて調べてみたらこんな感じでした。

• 従来の MySQL Connector/C は単体提供されない。サーバーをインストールしたらついてくる。
• MySQL Connector/C++ は、client/server protocol の方のAPIは C++ しか対応していない。 X Dev API の方は C 言語でも利用可能な形式になっている。

MySQL :: Download Connector/C (libmysqlclient)

ところで、Windows 版クライアントを考えたときに面白いのが MariaDB Connector/C です。 MySQL Connecotr/C が Windows 版でも OpenSSL 同梱になった等、 static link でシングルバイナリを作る使い方がほぼ不可能になったのに対して、 MariaDB Connector/C は OpenSSL を使わずに Windows の API を使って sha256_password や SSL 接続に対応していて、開発版では caching_sha2_password にも対応しました。

MySQL Connector/C がどうなってるのか公式のアナウンスがなかったのと、 MySQL Connector/Python が site-packages 直下に OpenSSL とか MySQL client の DLL とかをブッ込んでくる強硬手段を取ってきたので同じことをするとコンフリクトするので、 mysqlclient-python は Windows版の wheel の提供を中断していました。

MariaDB Connector/C であれば static link で DLL ブッコミ不要の従来どおりの提供ができそうなので、 caching_sha2_password 対応版が出たらそっちに切り替えるつもりでいます。

Python の repr(float) を高速化する

Python の repr(float) は、 float(repr(float)) が保証される最短の表現を返します。 実装には netlib の dtoa を使っています。

この dtoa より速い実装が世の中にはあって、 V8 なんかで使われているようです。

github.com

float の repr の実装をこの速いやつに入れ替えるパッケージが frepr です。 frepr.install() で float.__repr__ を入れ替えてしまうので、 json のエンコードなども速くすることができるらしい。

Python で float のデータを大量に扱う人には嬉しそう。

ISUCON 8 予選の Go 初期実装に見る初心者コード

会社のBlogにも書いたのですが、ISUCON 8 予選で負けてきました。

さて、 ISUCON の初期実装の定番として、初心者が書いたようなSQLやコードになっている点が挙げられます。

今回の Go の初期実装もその定番にもれず、初心者がやりがちな、Goの良さを殺してしまうコードがありました。

今回負けた反省点の一つとして、アプリの書き換えを二人でやっていたのでコンフリクトを恐れてそのリファクタリングを怠ったというのもあります。 Goで本戦に参加されるチームの方にはぜひこれを克服してもらいたいと思います。

アンチパターン: 長い無名関数

例: https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L404-L492

   e.GET("/api/users/:id", func(c echo.Context) error {
        var user User
        if err := db.QueryRow("SELECT id, nickname FROM users WHERE id = ?", c.Param("id")).Scan(&user.ID, &user.Nickname); err != nil {
            return err
        }

        loginUser, err := getLoginUser(c)
        if err != nil {
            return err
        }
        if user.ID != loginUser.ID {
            return resError(c, "forbidden", 403)
        }
... [以下数十行]

Sinatra 風のコードを移植しようとするとありがちなのですが、関数名が main.func8 とかになってしまって、Goの良さであるスタックトレースやプロファイルの使い勝手を大きく損ねます。Goでこういう無名関数の使い方はやめましょう。 (Ruby や JavaScript では良いのかという話もありますが、それはまた文化が違うので…)

例えば sort.Slice() にわたす比較関数のようにごく短く、中からDBアクセスなどの処理をしない関数は無名関数にしてもいいです。

また、１つの通常関数の中に無名関数が１つかせいぜい２つあるくらいなら、やはり害は少ないです。

スタックトレースとか flamegraph に main.func6 とか main.func8 とかがバラバラでるのはダメです。

リファクタリング: 通常関数への書き換え

無名関数全体を移動し、適当に名前をつけましょう。

ISUCON であれば事前練習で、なにか機械的な命名規則を決めてしまうと命名に時間を取られずに済みます。 例えば上の "/api/users/:id" を扱う関数であれば、 handle_GetApiUsersId とかで良いです。Goの慣習の命名規則（アンダースコア使わない、 API とか ID とかは Api, Id にしない）からは大きくハズレますが、 ISUCON では悩まず機械的に作業できる利点の方が大きいです。

アプリの書き換えを複数人でやる場合、このリファクタリングは書き換え範囲が大きくてコンフリクトが厳しいので、真っ先にえいっとやってしまうと良いと思います。

アンチパターン: for rows.Next() ループ内でのクエリ実行

例: https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L236-L252

   rows, err := db.Query("SELECT * FROM sheets ORDER BY `rank`, num")
    if err != nil {
        return nil, err
    }
    defer rows.Close()

    for rows.Next() {
...
        err := db.QueryRow("SELECT * FROM reservations WHERE event_id = ? AND sheet_id = ? AND canceled_at IS NULL GROUP BY event_id, sheet_id HAVING reserved_at = MIN(reserved_at)", event.ID, sheet.ID).Scan(&reservation.ID, &reservation.EventID, &reservation.SheetID, &reservation.UserID, &reservation.ReservedAt, &reservation.CanceledAt)

こんな感じで、 rows.Next() ループの中でクエリを実行したり、クエリの実行を含む関数を呼び出すと、複数のDBのコネクションを利用してしまいます。

そしてもっと悪いことに、他の prefork 型のアプリではプロセス数を適当に絞ることができるのに対して、GoでDBのコネクションプールを絞ろうとすると上のようなコードでデッドロックの原因になります。デッドロックを避けるためにコネクションプールの上限を設定しないと、たとえCPUが２コアしかないようなMySQLサーバーに対して数百コネクションから並列でクエリを投げてしまい、遅いクエリがどれか解らないとか、MySQL側がデッドロックを誤検出するとか、いろんなトラブルの原因になります。

for rows.Next() ループの中ではそのクエリの結果のフェッチだけを行い、その結果の各行に対する処理は改めて別のループに書きましょう。

アンチパターン: 長い関数での defer rows.Close()

https://github.com/isucon/isucon8-qualify/blob/9d7890f5433bdaf2cec75b4cdf1ebd0d9a531281/webapp/go/src/torb/app.go#L404-L492

上のアンチパターンの亜種ですが、せっかく for rows.Next() ループ内からDBにアクセスする処理を排除しても、 defer rows.Close() を使っていると、そのクエリに使われたコネクションは関数が終わるまでコネクションプールに返却されません。

とはいえ、安易に defer rows.Close() を for rows.Next() ループの後ろに rows.Close() の形で移動するのもダメです。 for rows.Next() ループ内に return が無いか確認しましょう。

リファクタリング: sqlx の利用

上のような問題の「まっとうな」リファクタリング方法は、１つのクエリを実行して結果をフェッチするまでを個別に関数に切り出すことです。そうすると defer rows.Close() が適切なタイミングで実行されます。

しかし ISUCON だといちいちそういったリファクタリングをしている余裕がないかもしれません。そこで sqlx を覚えておくと良いでしょう。

sqlx は、 database/sql の上位互換になっています。 sql.Open を sqlx.Open に書き換えるだけで、そのままのコードが動きます。

そして、 db.QueryRow(...).Scan(&data) を db.Get(&data, ...) に、 db.Query(...), for rows.Next() { var row Record; rows.Scan(&row); records = append(records, row) } のようなパターンを db.Select(&records, ...) に書き換える事ができます。

特に後者が強力で、 rows.Next() と rows.Close() を排除することができるので、コネクションを無駄に大量消費する問題を楽に解決することができます。

それ以外にも便利機能がいくつかあるので、 database/sql を直接使った初心者コードをリファクタリングするときの強力な武器として練習しておくことをおすすめします。