Day 014 — sync.Mutex / RWMutex（コードレビュー編）

📚 背景知識（読んでから問題へ）

前回（Day 013）はsync.Mutex/sync.RWMutexを自分でゼロから実装しました。今回は視点を変え、他人が書いたロック処理のレビューをします。実務でMutexが絡むバグは、書いた本人がその場で気づけないことがほとんどです。コンパイラは型エラーを検出してくれますが、「ロックの解放漏れ」「ロックの取り直しによるデッドロック」「構造体ごとのコピー」「保護されていない読み取り」はすべてコンパイルは通るのに実行時にだけ問題が起きるバグだからです。だからこそ、コードレビューでこれらのパターンを見抜く訓練が実務で強く役立ちます。

今回の問題は、Day 013の「よくある誤解・ミス」で扱った4つのパターンを、実際に動く（動かない）1つのプログラムに埋め込んだものです。

• ロックの解放漏れ: Lock()した後、エラーで早期returnする経路にUnlock()が書かれていない。deferを使わずに手動でUnlock()する設計は、分岐が増えるたびに漏れのリスクが上がる
• RLock保持中のLock取得（自己デッドロック）: RLock()している最中に、同じgoroutineが（別メソッド経由で間接的にでも）Lock()を呼ぶと、「自分が解放するはずのRLockを、自分が待っている」という状態に陥り永久にブロックする
• 構造体ごとの値渡し（Mutexのコピー）: sync.Mutex/sync.RWMutexを含む構造体を値として関数に渡すと、ロックの状態までコピーされる。コピーされたロックは元の変数のロックとは別物になり、排他制御が意味を失う（go vetのcopylocksがこれを検出できる）
• 保護されていない読み取り: 「読むだけだから安全」という思い込みで、ロックなしで直接mapを読む

これらは個別に見れば単純ですが、複数人が触るコードベースでは別々のメソッドに分散して混入するため、レビュー時に「このメソッドが呼ばれている間、他に何が起きているか」を頭の中で並行に追う訓練が必要です。

📝 問題

以下は、在庫を管理するInventory型のコードです。作者は「Mutex/RWMutexで排他制御したつもり」ですが、4箇所に別々の種類のバグがあります。

バグのあるコード:

📋 コピーpackage main

import (
	"fmt"
	"sync"
)

type Inventory struct {
	mu    sync.RWMutex
	stock map[string]int
}

func NewInventory() *Inventory {
	return &Inventory{stock: make(map[string]int)}
}

// Reserve は item を qty 個だけ予約（減算）する。
func (inv *Inventory) Reserve(item string, qty int) error {
	inv.mu.Lock()
	current, ok := inv.stock[item]
	if !ok {
		return fmt.Errorf("item not found: %s", item)
	}
	if current < qty {
		inv.mu.Unlock()
		return fmt.Errorf("insufficient stock: %s", item)
	}
	inv.stock[item] = current - qty
	inv.mu.Unlock()
	return nil
}

// CheckAndRestock は在庫が threshold を下回っていたら addQty 補充する。
func (inv *Inventory) CheckAndRestock(item string, threshold, addQty int) {
	inv.mu.RLock()
	defer inv.mu.RUnlock()
	if inv.stock[item] < threshold {
		inv.Restock(item, addQty)
	}
}

// Restock は item を qty 個補充する。
func (inv *Inventory) Restock(item string, qty int) {
	inv.mu.Lock()
	defer inv.mu.Unlock()
	inv.stock[item] += qty
}

// PrintSnapshot は在庫の状態を出力する。
func PrintSnapshot(inv Inventory) {
	inv.mu.RLock()
	defer inv.mu.RUnlock()
	fmt.Println(inv.stock)
}

// Peek は item の現在の在庫数だけを覗き見る（読み取り専用のつもり）。
func (inv *Inventory) Peek(item string) int {
	return inv.stock[item]
}

func main() {
	inv := NewInventory()
	inv.stock["widget"] = 10
	inv.stock["gadget"] = 3

	var wg sync.WaitGroup
	for i := 0; i < 20; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			if err := inv.Reserve("gizmo", 1); err != nil { // "gizmo" は在庫に存在しないアイテム
				return
			}
		}()
	}
	wg.Wait()

	inv.CheckAndRestock("gadget", 5, 20)

	fmt.Println("widget stock:", inv.Peek("widget"))
	PrintSnapshot(*inv)
}

要件:

1. コード中に埋め込まれた4つのバグをすべて特定してください。それぞれについて「どのメソッドの、どの行が問題か」「実行するとどのような不具合が起きるか（ハング/デッドロック・データ競合・panicのどれに繋がるか）」を説明してください
2. 4つのバグをすべて修正し、go run main.goが正常終了し、かつgo run -race main.goでdata raceが検出されない、完全な1つのプログラムを作成してください
3. 修正したコードに対してgo vet ./...（またはgo vet main.go）を実行し、修正前は検出されていた警告が消えていることを確認してください

go run main.go / go run -race main.go / go vet のいずれでも問題なく動作する、完全な1つのプログラムとして提出してください。

🔍 ヒント（段階的開示）

📋 コピーpackage main

import (
	"fmt"
	"sync"
)

type Inventory struct {
	mu    sync.RWMutex
	stock map[string]int
}

func NewInventory() *Inventory {
	return &Inventory{stock: make(map[string]int)}
}

func (inv *Inventory) Reserve(item string, qty int) error {
	inv.mu.Lock()
	defer inv.mu.Unlock() // TODO: これで全てのreturn経路をカバーできるか確認する
	// ... 以降、手動Unlock()は不要
	return nil
}

func (inv *Inventory) CheckAndRestock(item string, threshold, addQty int) {
	// TODO: RLockではなくLockを取る（書き込みうる関数だから）
}

func PrintSnapshot(inv *Inventory) { // TODO: ポインタ受け取りに変更
	// ...
}

func (inv *Inventory) Peek(item string) int {
	// TODO: RLock/RUnlockで保護する
	return 0
}

✅ 模範解答

📋 コピーpackage main

import (
	"fmt"
	"sync"
)

type Inventory struct {
	mu    sync.RWMutex
	stock map[string]int
}

func NewInventory() *Inventory {
	return &Inventory{stock: make(map[string]int)}
}

// Reserve は item を qty 個だけ予約（減算）する。
// Lock直後にdeferでUnlockすることで、どのreturn経路でも確実にロックが解放される。
func (inv *Inventory) Reserve(item string, qty int) error {
	inv.mu.Lock()
	defer inv.mu.Unlock()

	current, ok := inv.stock[item]
	if !ok {
		return fmt.Errorf("item not found: %s", item)
	}
	if current < qty {
		return fmt.Errorf("insufficient stock: %s", item)
	}
	inv.stock[item] = current - qty
	return nil
}

// CheckAndRestock は在庫が threshold を下回っていたら addQty 補充する。
// 書き込みが起こりうるため、最初からLock（書き込みロック）を取る。
// RLockからLockへの「昇格」はsync.RWMutexには存在しないため、
// 内部で別の書き込みロックメソッドを呼ぶ設計は避ける。
func (inv *Inventory) CheckAndRestock(item string, threshold, addQty int) {
	inv.mu.Lock()
	defer inv.mu.Unlock()
	if inv.stock[item] < threshold {
		inv.stock[item] += addQty
	}
}

// Restock は item を qty 個補充する（CheckAndRestockとは独立した公開API）。
func (inv *Inventory) Restock(item string, qty int) {
	inv.mu.Lock()
	defer inv.mu.Unlock()
	inv.stock[item] += qty
}

// PrintSnapshot は在庫の状態を出力する。
// ポインタで受け取ることで sync.RWMutex のコピーを避ける。
func PrintSnapshot(inv *Inventory) {
	inv.mu.RLock()
	defer inv.mu.RUnlock()
	fmt.Println(inv.stock)
}

// Peek は item の現在の在庫数を読み取る。読み取りもRLock/RUnlockで保護する。
func (inv *Inventory) Peek(item string) int {
	inv.mu.RLock()
	defer inv.mu.RUnlock()
	return inv.stock[item]
}

func main() {
	inv := NewInventory()
	inv.stock["widget"] = 10
	inv.stock["gadget"] = 3

	var wg sync.WaitGroup
	for i := 0; i < 20; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			if err := inv.Reserve("gizmo", 1); err != nil {
				return
			}
		}()
	}
	wg.Wait()

	inv.CheckAndRestock("gadget", 5, 20)

	fmt.Println("widget stock:", inv.Peek("widget"))
	PrintSnapshot(inv)
}

widget stock: 10
map[gadget:23 widget:10]

※ gizmoは在庫に存在しないため20回のReserveはすべてinsufficientではなくnot foundエラーで失敗し在庫は変化せず、gadgetは初期値3が閾値5を下回るため20補充されて23になります。go run -race main.goでもdata race検出なし、go vet main.goでも警告なしを確認済みです。

🪜 Step-by-Step 解説

📋 コピーfunc (inv *Inventory) CheckAndRestock(item string, threshold, addQty int) {
	inv.mu.RLock()
	defer inv.mu.RUnlock()
	if inv.stock[item] < threshold {
		inv.Restock(item, addQty) // Restock内部でinv.mu.Lock()を呼ぶ
	}
}

📋 コピーfunc PrintSnapshot(inv Inventory) { // 値渡し
	inv.mu.RLock()
	// ...
}

// 呼び出し側
PrintSnapshot(*inv)

📋 コピーfunc (inv *Inventory) Peek(item string) int {
	return inv.stock[item] // ロックなし
}

💡 設計思想・なぜこう書くのか

🌐 他言語との比較

Rustの所有権システムと比べると、Goのsync.Mutexはいずれのバグもコンパイラではなくgo vetや実行時観測（ハング・-race・fatal error）で気づくという設計になっていることが際立ちます。Goが「言語をシンプルに保つ」ために所有権チェッカーのような重厚な静的検証機構を持たない代償として、こうしたロック関連バグの発見は開発者の規律（deferの徹底、ロックスコープをメソッド内に閉じる、ポインタレシーバの徹底）とgo vet/-raceのようなツールに委ねられています。

🏆 実務での使いどころ

• コードレビューのチェックリスト化: 「Lock()の直後にdefer Unlock()があるか」「ロック保持中に他のpublicメソッドを呼んでいないか」「Mutexを含む構造体を値で渡していないか」は、レビュー時に機械的にチェックできる観点としてチームのレビュー基準に組み込む価値がある
• CIでのgo vet / go test -raceの常時実行: 今回のバグ③（copylocks）はgo vetで、バグ④のようなデータ競合はgo test -raceでCI上検出できる。人間のレビューだけに頼らず、ツールで機械的に検出できる部分は必ずパイプラインに組み込む
• ロックを取得するメソッドの命名規約: 「このメソッドは内部でロックを取得する」ことが名前から分かりにくいと、バグ②のような「知らずにロック保持中に別のロック取得メソッドを呼ぶ」事故が起きやすい。社内ではxxxLocked（呼び出し側で既にロック済みを前提とする内部ヘルパー）と、通常の公開メソッドを命名で区別するチームもある

⚠️ よくある誤解・ミス

🚀 次のステップ

• 発展: 今回のCheckAndRestockを、Restockを呼び出す設計のまま安全にするにはどうすればよいか考えてみましょう（ヒント: Restockのロック取得部分をrestockLockedという「呼び出し側が既にLock済みであることを前提とする」非公開ヘルパーに切り出し、公開版のRestockとCheckAndRestockの両方からそれを呼ぶ設計にすると、ロックの二重取得を構造的に避けられます）
• 次回予告: Day 015 — sync.Once, sync.Pool（実装問題）。初期化を一度だけ実行する仕組みと、オブジェクトの再利用によるGC負荷軽減を学びます

🎯 自己評価

自分の回答

気づき・メモ