Go Wiki: コードレビュー: Go並行処理

このページはGoコードレビューコメントリストへの追加です。このリストの目的は、Goコードをレビューする際に並行処理関連のバグを発見するのに役立つことです。

記憶をリフレッシュし、これらの並行処理の落とし穴をすべて認識していることを確認するために、このリストを一度読んでみてもよいでしょう。

⚠️ このページはコミュニティによって執筆・維持されています。議論されている情報、誤解を招く可能性のある情報、または不正確な情報が含まれている場合があります。

不十分な同期と競合状態

テスト

スケーラビリティ

時間

不十分な同期と競合状態

# RC.1. HTTPハンドラ関数は複数のゴルーチンから並行して呼び出しても安全ですか？ HTTPハンドラは、通常プロジェクトコード内のどこからも明示的に呼び出されず、HTTPサーバーの内部からのみ呼び出されるため、スレッドセーフであるべきことを見落としがちです。

# RC.2. プリミティブ型または明示的にスレッドセーフではない型（atomic.Valueなど）のフィールドまたは変数が、競合するゴルーチンから更新される可能性があるにもかかわらず、ミューテックスで保護されていないフィールドまたは変数アクセスはありますか？非アトミックなハードウェア書き込みや潜在的なメモリ可視性の問題があるため、プリミティブ変数であっても読み取りの同期をスキップすることは安全ではありません。

「Typical Data Race: Primitive unprotected variable」も参照してください。

# RC.3. スレッドセーフな型のメソッドが、保護された構造体へのポインタを返しませんか？ これは、前の項目で説明した保護されていないアクセス問題につながる微妙なバグです。例

type Counters struct {
    mu   sync.Mutex
    vals map[Key]*Counter
}

func (c *Counters) Add(k Key, amount int) {
    c.mu.Lock()
    defer c.mu.Unlock()
    count, ok := c.vals[k]
    if !ok {
        count = &Counter{sum: 0, num: 0}
        c.vals[k] = count
    }
    count.sum += amount
    count.n += 1
}

func (c *Counters) GetCounter(k Key) *Counter {
    c.mu.Lock()
    defer c.mu.Unlock()
    return c.vals[k] // BUG! Returns a pointer to the structure which must be protected
}

考えられる解決策の1つは、GetCounter()で構造体へのポインタではなく、コピーを返すことです。

type Counters struct {
    mu   sync.Mutex
    vals map[Key]Counter // Note that now we are storing the Counters directly, not pointers.
}

...

func (c *Counters) GetCounter(k Key) Counter {
    c.mu.Lock()
    defer c.mu.Unlock()
    return c.vals[k]
}

# RC.4. sync.Mapを更新できるゴルーチンが複数ある場合、以前のm.Load()呼び出しの成功に応じてm.Store()またはm.Delete()を呼び出していませんか？ 言い換えれば、次のコードは競合状態にあります

var m sync.Map

// Can be called concurrently from multiple goroutines
func DoSomething(k Key, v Value) {
    existing, ok := m.Load(k)
    if !ok {
        m.Store(k, v) // RACE CONDITION - two goroutines can execute this in parallel
        ... some other logic, assuming the value in `k` is now `v` in the map
    }
    ...
}

このような競合状態は、場合によっては無害である可能性があります。例えば、Load()とStore()の呼び出しの間のロジックがマップにキャッシュされる値を計算し、この計算は常に同じ結果を返し、副作用がない場合などです。

⚠️ 誤解を招く可能性のある情報。「競合状態」は、この例のように無害である可能性のあるロジックエラーを指すことがあります。しかし、このフレーズは、決して無害ではないメモリモデルの違反を指すこともよくあります。

競合状態が無害でない場合は、sync.Map.LoadOrStore()メソッドとLoadAndDelete()メソッドを使用して修正してください。

スケーラビリティ

# Sc.1. make(chan *Foo)のように、チャネルが容量ゼロで作成されているのは意図的ですか？容量ゼロのチャネルにメッセージを送信するゴルーチンは、別のゴルーチンがこのメッセージを受信するまでブロックされます。make()呼び出しで容量を省略することは、単なる間違いである可能性があり、コードのスケーラビリティを制限し、おそらくユニットテストではそのようなバグは見つからないでしょう。

⚠️ 誤解を招く情報。バッファリングされたチャネルは、本質的にバッファリングされていないチャネルと比較して「スケーラビリティ」を向上させません。しかし、バッファリングされたチャネルは、バッファリングされていないチャネルではすぐに明らかになるであろうデッドロックやその他の根本的な設計エラーを容易に隠す可能性があります。

# Sc.2. RWMutexを使用したロックは、プレーンなsync.Mutexと比較して余分なオーバーヘッドを伴い、さらに、GoにおけるRWMutexの現在の実装にはスケーラビリティの問題がある可能性があります。ケースが非常に明確な場合（例えば、数百ミリ秒以上続く多くの読み取り専用操作を同期するためにRWMutexが使用され、排他的ロックを必要とする書き込みがめったに発生しない場合など）を除いて、RWMutexが実際にパフォーマンス向上に役立つことを証明するベンチマークが存在すべきです。 RWMutexが明らかに良いことよりも害を及ぼす典型的な例は、構造体内の変数の単純な保護です。

type Box struct {
    mu sync.RWMutex // DON'T DO THIS -- use a simple Mutex instead.
    x  int
}

func (b *Box) Get() int {
    b.mu.RLock()
    defer b.mu.RUnlock()
    return b.x
}

func (b *Box) Set(x int) {
    b.mu.Lock()
    defer b.mu.Unlock()
    b.x = x
}

時間

# Tm.1. time.Tickerはdefer tick.Stop()を使って停止されていますか？ ループでティッカーを使用する関数が戻る際にティッカーを停止しないと、メモリリークになります。

# Tm.2. time.Time構造体は==だけでなく、Equal()メソッドを使って比較されていますか？ time.Timeのドキュメントを引用します。

Goの==演算子は、時刻の瞬間だけでなく、ロケーションと単調クロックの読み取りも比較することに注意してください。したがって、Time値は、すべての値に対して同じロケーションが設定されていることを最初に保証することなく（これはUTC()またはLocal()メソッドを使用することで達成できます）、またt = t.Round(0)を設定することで単調クロックの読み取りが取り除かれていることを保証することなく、マップまたはデータベースキーとして使用すべきではありません。一般的に、t.Equal(u)は、利用可能な最も正確な比較を使用し、引数のいずれか一方のみが単調クロックの読み取りを持つ場合を正しく処理するため、t == uよりも優先すべきです。

# Tm.3. time.Since(t)を呼び出す前に、単調な要素はtから取り除かれていませんか？ これは前の項の結果です。time.Since()関数に渡す前に（UTC()、Local()、In()、Round()、Truncate()、またはAddDate()を呼び出すことで）time.Time構造体から単調な要素が取り除かれている場合、time.Since()の結果は負になる可能性があります。これは非常にまれなケースで、例えば、開始時刻が最初に取得された時点とtime.Since()が呼び出された時点の間でシステム時刻がNTPを介して同期された場合などです。単調な要素が取り除かれていない場合、time.Since()は常に正の期間を返します。

# Tm.4. t.Before(u)でシステム時刻を比較したい場合、引数から単調な要素を取り除いていますか（例：u.Round(0)を使用）？これはTm.2に関連するもう1つの点です。特定の状況では、2つのtime.Time構造体を、それらに格納されているシステム時刻のみで比較する必要があります。これらのTime構造体のいずれかをディスクに保存したり、ネットワーク経由で送信したりする前にこれが必要になる場合があります。例えば、テレメトリメトリックを時刻とともに定期的にリモートシステムにプッシュする何らかのテレメトリエージェントを想像してみてください。

var latestSentTime time.Time

func pushMetricPeriodically(ctx context.Context) {
    t := time.NewTicker(time.Second)
    defer t.Stop()
    for {
        select {
        case <-ctx.Done: return
        case <-t.C:
            newTime := time.Now().Round(0) // Strip monotonic component to compare system time only
            // Check that the new time is later to avoid messing up the telemetry if the system time
            // is set backwards on an NTP sync.
            if latestSentTime.Before(newTime) {
                sendOverNetwork(NewDataPoint(newTime, metric()))
                latestSentTime = newTime
            }
        }
    }
}

このコードは、Round(0)を呼び出さない、つまり単調な要素を取り除かないと間違っています。

参考資料

Goコードレビューコメント: Goコードをレビューするためのチェックリストで、並行処理に特化したものではありません。

Goの並行処理

並行処理（Goに特化しない）

このコンテンツはGo Wikiの一部です。

go.dev は、サービスの品質を提供および向上させ、トラフィックを分析するために、Google の Cookie を使用しています。詳細はこちらをご覧ください。