はじめに
アドベントカレンダー初参加です！
とある企業でサーバーサイドエンジニアとして働いており、主にGoでAPIを実装しています。
今年に入って新規事業の開発を任され、色々やらかしを経験させていただきました。
その中でも一番のやらかしを自戒の念も込めて投稿したいと思います。
特定を避けるために敢えて分かりづらく表現している箇所があります。その点ご了承くださいmm
惨劇発覚前夜
とある会員制サイトのリリースを控えており、それに備えて色々準備を整えてました。
リリース当日はそれなりにアクセス急増が見込まれるので
- ALBの暖気申請
- フロントエンドサーバーのスケールアウト
等の対応を行いました。
今までも似たようなサイトをいくつかリリースしており、上記の対応でアクセスは捌けていたので今回も同様の対応で問題ないと思ってました。
リリース当日PM20:00 にサイトのURLを公開。
今までをはるかに上回るアクセスが来ましたが、特にサイトの表示につまることはなく、当日のリリース作業を終えることができました。
惨劇発覚
次の日、朝起きるとサポート用メアドに何件かメールが届いていました。
恐る恐る確認すると、重複してはいけないはずのユーザー情報が重複しているとの問い合わせが。。
すぐにマネージャーに報告し、原因究明に取り掛かりました。
惨劇はなぜおこってしまったのか
APIログを確認したところ、会員登録トランザクションでデッドロックエラーが数件吐かれてました。
DB（mysql）にてSHOW ENGINE INNODB STATUS;を実行したところ、会員登録トランザクションの以下のクエリ間でデッドロックが発生しているようでした。
- 会員テーブルから会員情報を取得
- 会員情報のステータスを修正し、UPDATE
ところが仮にデッドロックが発生したとしても、アプリケーション側でちゃんとエラーハンドリングされロールバックされれば、今回のような大きな障害には繋がりません。
今回はまさに上記の1の処理でエラーハンドリングを怠っていたために発生した障害でした。
簡単にコードで再現すると以下のような実装でした。
func (r *repo) GetMember(ctx context.Context, id uint64) (member *Member, err error) {
// ORMでクエリ構築(省略)
// クエリ実行
if _, err := db.LoadContext(ctx, member); err != nil {
err = xerrors.Errorf(": %w", err)
}
return
}
Goでは関数の戻り値に名前をつけて、関数内で扱うことができます。
上記ではクエリ実行のエラーがnilでなかった場合、エラーをラップして関数の名前付き戻り値errを上書きし、最後にreturnするという意図の実装でした。
しかし、実際には、errがif文内で新たに宣言され、関数の名前付き戻り値とは別の変数として新しく宣言されてしまっています。
// `:=`ではなく
if _, err := db.LoadContext(ctx, member); err != nil
// `=`が正解
if _, err = db.LoadContext(ctx, member); err != nil
この結果、クエリ実行時の変数errのスコープはif文内に限定され、エラーが握り潰されてしまいました。
そのため1の処理ではエラーが返されず、そのまま2の処理に進み、トランザクションがコミットされて不整合のあるデータが生まれてしまった、というオチです。
二度と惨劇を起こさないためにどうしたのか
主に以下のような対応を行いました。
- テーブル内で重複させたくないカラムには
UNIQUE KEYを付与する- なぜこれを事前にやってなかったかは過去の自分に問いたいです
- エラーが発生した場合は、そのままreturnすることを基本とする
- エラー判定のif文でreturnすることを徹底すれば、このようなミスが防げるのではという考えです
- ただ書き方が冗長になる場合もあると思うので、あくまで意識として。。
- エラーハンドリングのミスを機会的に防ぐために、Goの静的解析
errcheckなども試してみましたが、今回のケースはアラートとして出してくれませんでした。。
- エラー判定のif文でreturnすることを徹底すれば、このようなミスが防げるのではという考えです
if _, err := db.LoadContext(ctx, member); err != nil {
err = xerrors.Errorf(": %w", err)
return
}
- ユーザー登録や決済など、クリティカルな処理は負荷テストを行うようにする
- 今まで、同様の実装で何件かサイトのリリースを行いましたが、今回はそれまでを遥かに上回るアクセスが来たため、このようにバグとして明るみに出る結果となりました
- まずはローカルでバグを再現する必要があると思い、Goを並列でテストする
t.Parallelメソッドを使って、500件同時に会員登録を行うシナリオテストを実装しました- この負荷テストの詳細に関してはまた別の機会に紹介できればと思います
- その結果ローカルで再現でき、バグフィックス後の確認も安全に行うことができました
終わりに
この一年色々やらかして、関係者の皆様には申し訳なさでいっぱいです。
それでも今の環境で応援していただけているのは感謝しかありません。
今後もサービスと共に成長できるように精進します。
P.S.
名前付き戻り値って便利な反面、少しスコープが分かりづらいなと思いました。
だからと言って、コード規約として全面禁止にするというのも踏み込めず、他社さんではどのように扱っているのか気になっております。
名前付き戻り値や今回のエラーハンドリングの扱い、その他諸々に関してアドバイスいただけますと大変嬉しいです。。！