Goのtime.Afterをループ内で使う実装をどうレビューするか

Goの time.After は、一定時間後に通知を受け取る簡単なAPIだ。
短い待機やtimeoutには便利だが、ループ内で繰り返し呼ぶ実装はレビューで慎重に見る必要がある。

特に危ないのは次のような構造だ。

  • for ループ内の select で毎回 time.After を作る
  • 周期実行なのに time.After で代用している
  • context キャンセル時にTimerの責務が読めない
  • timeout、interval、retry waitが同じ書き方になっている

この記事では time.After を禁止するのではなく、
時間制御の目的とライフサイクルが読めるかをレビューする観点を整理する。

まず止めたい実装

ループ内でtime.Afterを作り続ける実装 
func StartPolling(ctx context.Context, jobs <-chan Job) {
    for {
        select {
        case <-ctx.Done():
            return

        case job := <-jobs:
            handle(job)

        case <-time.After(5 * time.Second):
            refresh()
        }

@Reviewer
ループのたびに `time.After` で新しいTimerを作っています。周期実行が目的なら `time.NewTicker` を使い、停止責務を明示してください。
    }
}

このコードは、5秒ごとに refresh() したい意図に見える。
しかし実装としては、selectに入るたびに新しいTimerを作っている。

レビューでは「5秒待てるか」だけではなく、
時間制御オブジェクトを誰が作り、誰が止めるかを見る必要がある。

なぜ危ないのか

time.After(d) は、内部的にはTimerを作り、そのチャネルを返す。
単発のtimeoutなら分かりやすいが、ループ内で頻繁に呼ぶと、時間制御の意図が曖昧になる。

実務では次の問題につながる。

  • 周期実行なのか、アイドル時timeoutなのか分からない
  • Timer生成が過剰になり、負荷の原因になる
  • キャンセル時に待機責務が読みにくい
  • テストで時間を制御しにくい
  • 将来intervalを変更したとき影響範囲が追いにくい

レビューでは、time.After が書かれている箇所で
単発の待機なのか、継続的な周期なのかを必ず確認したい。

レビューで見たい3つの判断線

1. 周期実行ならTickerになっているか

5秒ごと、1分ごとのような周期実行は、time.NewTicker のほうが意図を表しやすい。

Comment 
@Reviewer: この処理は一定間隔の周期実行に見えます。`time.After` をループ内で都度作るのではなく、`time.NewTicker` を使ってintervalと停止責務を明示してください。

2. 単発timeoutなら範囲が狭いか

time.After が合うのは、1回の処理に対するtimeoutのような場面だ。

単発timeoutとして自然な例 
select {
case result := <-resultCh:
    return result, nil
case <-time.After(3 * time.Second):
    return Result{}, ErrTimeout
}
Comment 
@Reviewer: この `time.After` は単発の待機境界として使われており、ループ内でTimerを作り続ける構造ではありません。timeoutの値が呼び出し元の期待と一致しているかだけ確認してください。

3. ctx.Done() と時間制御の優先順位が読めるか

時間待ちとキャンセルが同時にある場合、キャンセルで止まれることが重要だ。

Comment 
@Reviewer: 時間待ちの処理はありますが、`ctx.Done()` で停止できる構造か確認してください。呼び出し元が処理を終了させられない時間制御はgoroutineリークにつながります。

改善例：Tickerで周期を表す

周期実行なら、Tickerを作って停止する。

Tickerで周期実行を表す実装 
func StartPolling(ctx context.Context, jobs <-chan Job) {
    ticker := time.NewTicker(5 * time.Second)
    defer ticker.Stop()

    for {
        select {
        case <-ctx.Done():
            return

        case job, ok := <-jobs:
            if !ok {
                return
            }
            handle(job)

        case <-ticker.C:
            refresh()
        }
    }
}

この構造なら、レビューアーは次を確認しやすい。

  • 周期実行であることがAPIから読める
  • tickerの停止責務がある
  • ctx キャンセルで抜けられる
  • jobs closeでも終了できる
  • intervalの変更箇所が明確

retry waitと周期実行を混ぜない

リトライ待ちでも time.After はよく出てくる。
ただし、周期実行とリトライ待ちは意味が違う。

リトライ待ちが埋もれている例 
for {
    if err := syncOnce(ctx); err == nil {
        return nil
    }

    select {
    case <-ctx.Done():
        return ctx.Err()
    case <-time.After(10 * time.Second):
    }
}

この場合、time.After は周期実行ではなく「失敗後の待機」である。
レビューでは、待機時間の根拠、最大試行回数、呼び出し元のtimeoutと整合しているかを見る。

Comment 
@Reviewer: この `time.After` はリトライ待機として使われていますが、最大試行回数と全体timeoutが見えません。呼び出し元のctx期限内で収まる設計か、retry policyとして切り出してください。

レビュー観点チェックリスト

time.Afterレビューの確認項目
  • ループ内で time.After を作り続けていないか
  • 周期実行なら time.NewTicker を使っているか
  • TickerやTimerの停止責務が明示されているか
  • 単発timeout、周期実行、リトライ待機が混ざっていないか
  • ctx.Done() で待機を中断できるか
  • 待機時間が呼び出し元のSLAやtimeoutと整合しているか

まとめ

time.After は単発の待機には分かりやすい。
しかし、ループ内で使うと、周期実行、リトライ待機、timeoutの意味が混ざりやすい。

レビューでは、time.After そのものではなく、
時間制御の目的と停止責務がコードから読めるかを確認したい。