deferで「エラーが出ていたらロールバック」をするのをやめよう
導入
Go ではerrという名前の変数を使ってエラーハンドリングをするのが一般的です。
一般的であるがためにerrを不用意に使ってしまうと意図しない挙動になりかねません。
例: errでトランザクションを管理する実装
DB アクセスのある関数で、以下の要件を満たす実装を行いました。
- 関数のどこかでエラーになったら トランザクションをロールバックする
- エラーにならなければトランザクションをコミットする
※実装は簡略化しています
var err error
tx, err := db.Begin()
if err != nil {
return err
}
defer func() {
if err != nil {
tx.Rollback()
} else {
tx.Commit()
}
}()
value, err := anyFunc()
if err != nil {
return err
}
return nil
トランザクション中に実行される anyFunc の結果によって トランザクションをコミットするか決定しています。
anyFunc から返ったエラーは最初に定義したerrに代入されます。
それがnilでなければロールバックが行われ、そうでなければコミットされます。
この時点では要件通りで問題ありませんが、保守性を考えると望ましい実装ではありません。
変更して壊れる例
以下は簡単な修正を加えたことで意図しないコミットが行われるようになってしまう例です。
var err error
tx, err := db.Begin()
if err != nil {
return err
}
defer func() {
if err != nil {
tx.Rollback()
} else {
tx.Commit()
}
}()
if anyCondition { // anyFuncをある条件下でのみ実行するようにした
value, err := anyFunc()
if err != nil {
return err
}
}
return nil
最初の例と違う部分は anyFunc とそのエラー処理を if ブロックの中で実行するようにした点だけですが、anyFunc からエラーが返った場合にロールバックではなくコミットが行われるようになりました。
一見問題なく見える単純な変更で、重大なバグが発生してしまいました。
この問題には Go の代入の仕様が関係しています。
:=を使った代入とシャドーイング
Go では:=を使うことで新しい変数を宣言しながら代入することが出来ます。
単一の変数に対する代入
同じスコープに同名の変数が存在したとしても、同じブロックに別の変数が作成されます。（シャドーイング）
既に同じブロックで同名の変数が宣言されている場合はエラーになります。
複数の変数に対する代入
代入される変数のうち、未宣言ものは変数の作成と代入が行われ、宣言済みのものには代入だけが行われます。
ただし、代入される変数が全て同じブロックで宣言済みの場合はエラーになります。
なぜ壊れたのか
anyFuncのエラーが最初に定義したerrに代入されず、新しく宣言された if の中のerrに代入されることになります。
defer の中でアクセスされるerrはデフォルト値のnilのままで、anyFuncがエラーであるかにかかわらず常にコミットするようになってしまいました。
改善策
1. ロールバック用のフラグを新しく定義する
var err error
needsRollback := true
tx, err := db.Begin()
if err != nil {
return err
}
defer func() {
if needsRollback {
tx.Rollback()
} else {
tx.Commit()
}
}()
if anyCondition {
value, err := anyFunc()
if err != nil {
return err
}
}
needsRollback = false
return nil
概要
- ロールバックが必要であることを示すフラグ(
needsRollback)を追加する -
trueであればロールバックする
メリット
-
err以外にコミット/ロールバックの条件がある場合に便利 - フラグを変更しない場合ロールバックなので、異常系の実装漏れがおこりづらい
デメリット
- 全ての正常系にフラグを
falseにする処理を追加する必要がある
2. トランザクション管理を外に出す
// トランザクション管理以外の処理
func process(tx Tx) error {
if anyCondition {
value, err := anyFunc()
if err != nil {
return err
}
}
return nil
}
var err error
tx, err := db.Begin()
if err != nil {
return err
}
if err := process(tx) {
tx.Rollback()
} else {
tx.Commit()
}
概要
- トランザクションの開始と終了を関数外に切り分ける
メリット
- トランザクションの状態を気にせず実装できる
- コミットするフローが複数あっても追加の記述がない
- テストや再利用がしやすい
デメリット
- 既存の実装によっては設計の変更が難しい場合がある
3. 常にロールバックする
var err error
tx, err := db.Begin()
if err != nil {
return err
}
defer tx.Rollback() // コミットしていたら何も起こらない
if anyCondition {
value, err := anyFunc()
if err != nil {
return err
}
}
tx.Commit() // 処理が成功したらコミットする
return nil
概要
- 処理が成功したかにかかわらずロールバックする
- 多くのライブラリはコミット後のロールバックで何も起こらないため
メリット
- 最も単純
- フラグを変更しない場合ロールバックなので、異常系の実装漏れがおこりづらい
デメリット
- 処理が失敗したときにロールバック以外にすることがある場合採用できない
- コミット後のロールバックがエラーになるライブラリでないか確認する必要がある
終わりに
errを代入直後以外で使うのを止めよう！
Discussion
goplsのanalysesのshadowを有効にするとshadowingがすべてwarningにできますのでそっちでやる+CIでチェックするという方針でもありかと思います。
コメントありがとうございます。
公式のツールでチェック出来るのは取り入れやすくて良いですね。
書いといてなんですが、shadowingがwarningになるのはかなりverboseなので私は切ってます。
そして確かにNoboNoboさんのおっしゃっている通り、named return valueを使ったほうがいいですね！
named return valueを使うと、returnした値がnamed return valueに代入されたみたいにふるまいますので、かなりこの事故は防げます。
適当なサンプルを作って
objdump -dして中身を確認してみたところ上記の説明であってる挙動をしていましたが、NoboNoboさんの示されたサンプルで過不足なさそうなのといい例が思いつかなかったから変になってしまったのでdetialsに隠しておきます。objdumpで見たnamed return valueの挙動
4981c0,4981c4でarg <= 5の比較、4981c6で6をdeferで呼び出してる無名関数のキャプチャーしてる変数に代入,4981dcでdeferされた関数実行。4981deでキャプチャーされた値を%rax(引数と返り値はどちらもレジスタ渡しをするので返り値)に再度格納、今回は特に値を変更していないが場合によっては数値が変わっている。4981e8でreturn。上で書いたことの認識は当たってそうです。
やっぱり使いどころは難しいですよね。でも曖昧な部分が減るのは代えがたいメリットだと思います。
挙動の確認も勉強になりました。ありがとうございます！
冒頭の書き方の
var err errorと返値のerrorは別の物を指しているので、err変数と実際に返されるerrorの値が異なることが起きるので推奨されません。
こういう時は名前付き返値を使うと良いです。
この場合、関数の冒頭でシャドウイング前に参照するerrシンボルがreturnに渡されたものであることが保証されます。
（その後シャドウイングが発生しても大丈夫）
これはかなり綺麗に書けて良いですね！
記事を書く前にシャドーイングについて調べたのですが全く気づきませんでした…