Goのhttp.Clientでtimeout未設定の実装をどうレビューするか
Goのhttp.Clientでtimeout未設定の実装をどうレビューするか
Goで外部APIを呼び出すコードでは、http.Client を作って Do すれば通信できる。
しかしレビューで見落としやすいのが、timeoutがどこにも設定されていない実装である。
危ないのは次のようなコードだ。
http.Getやhttp.Postをそのまま使っている&http.Client{}にTimeoutがないcontext.Contextを受け取っているのにrequestへ渡していない- リトライはあるが、全体の時間予算がない
- 長時間接続なのか単なる設定漏れなのか説明がない
この記事ではHTTPクライアント設計全体ではなく、
外部呼び出しがいつ諦めるのかをレビューで確認する観点を整理する。
まず止めたい実装
func FetchInvoice(userID string) (*Invoice, error) {
resp, err := http.Get("https://api.example.com/invoices/" + userID)
if err != nil {
return nil, err
}
defer resp.Body.Close()
var invoice Invoice
if err := json.NewDecoder(resp.Body).Decode(&invoice); err != nil {
return nil, err
}
return &invoice, nil
}@Reviewer: `http.Get` は呼び出し側でtimeoutを制御できません。外部APIが応答しない場合に処理が待ち続けるため、timeout付きの `http.Client` と `context` を使う構造にしてください。このコードは正常系では短く見える。
しかし外部APIが遅延したとき、呼び出し元のgoroutineやworkerを長く占有する。
レビューでは、リクエストが送れるかだけでなく、
応答が返らないときにどの責務が処理を止めるのかを見る必要がある。
なぜ危ないのか
timeout未設定の問題は、テストでは露出しにくい。
本番で下流サービス、DNS、ネットワーク、プロキシのどこかが詰まったときに初めて効いてくる。
起きやすい問題は次の通りである。
- goroutineやworkerが外部応答待ちで滞留する
- 上位APIのSLAより長く待ってしまう
- リトライと組み合わさって総待ち時間が膨らむ
- キャンセル済みリクエストが下流呼び出しを続ける
- 障害時にどの層が諦めたのかログから読めない
外部APIは失敗するだけでなく、返ってこないことがある。
レビューでは、失敗時のエラー処理より前に、待ち時間の境界を確認したい。
レビューで見たい3つの判断線
1. http.Client の生成責務が散らばっていないか
各関数で &http.Client{} を作ると、timeout設定が抜けやすい。
外部サービスごとにclientを構成し、呼び出し側へ注入する方がレビューしやすい。
func NewInvoiceClient(baseURL string) *InvoiceClient {
return &InvoiceClient{
baseURL: baseURL,
client: &http.Client{
Timeout: 5 * time.Second,
},
}
}@Reviewer: 関数内で `&http.Client{}` を都度生成しているため、timeout設定が呼び出しごとにばらつきます。外部API単位のclient生成箇所に集約してください。2. context がrequestへ伝播しているか
上位リクエストがキャンセルされたら、下流のHTTP呼び出しも止めたい。
context.Context を受け取っていても、http.NewRequestWithContext を使っていなければ伝播しない。
@Reviewer: `ctx` を受け取っていますが、HTTP requestに渡していません。呼び出し元のキャンセルやdeadlineが外部API呼び出しへ伝播するよう、`http.NewRequestWithContext` を使ってください。3. リトライ込みの時間予算があるか
1回あたりのtimeoutがあっても、リトライ回数が多ければ総時間は膨らむ。
レビューでは、単発のtimeoutと全体のdeadlineを分けて確認する。
@Reviewer: 1回のHTTP timeoutはありますが、3回リトライした場合の総待ち時間が呼び出し元のSLAを超える可能性があります。全体deadlineとリトライ間隔を合わせて設計してください。timeoutは値そのものより、上位の時間予算と一致していることが重要である。
改善例
client生成、context伝播、ステータス判定を明示する。
type InvoiceClient struct {
baseURL string
client *http.Client
}
func NewInvoiceClient(baseURL string, timeout time.Duration) *InvoiceClient {
return &InvoiceClient{
baseURL: baseURL,
client: &http.Client{
Timeout: timeout,
},
}
}
func (c *InvoiceClient) FetchInvoice(ctx context.Context, userID string) (*Invoice, error) {
req, err := http.NewRequestWithContext(
ctx,
http.MethodGet,
c.baseURL+"/invoices/"+url.PathEscape(userID),
nil,
)
if err != nil {
return nil, fmt.Errorf("create invoice request: %w", err)
}
resp, err := c.client.Do(req)
if err != nil {
return nil, fmt.Errorf("fetch invoice: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096))
return nil, fmt.Errorf("fetch invoice: status=%d body=%q", resp.StatusCode, string(body))
}
var invoice Invoice
if err := json.NewDecoder(resp.Body).Decode(&invoice); err != nil {
return nil, fmt.Errorf("decode invoice response: %w", err)
}
return &invoice, nil
}この構造なら、レビューアーは次を確認しやすい。
- timeoutがclient生成時に決まっている
- 呼び出し元の
ctxがrequestに伝播している - userIDがURL pathとしてescapeされている
- HTTP失敗とdecode失敗の文脈が分かれている
- client生成箇所を見れば外部APIの待ち時間方針が読める
timeoutを付けない例外は説明が必要
Server-Sent Events、long polling、大きなファイルのstreamingなど、単純な短時間timeoutに向かない通信もある。
その場合でも、レビューでは「timeoutなしでよい」とは扱わない。
@Reviewer: このHTTP呼び出しは長時間接続に見えます。`http.Client.Timeout` を一律に設定しない判断はあり得ますが、context deadline、read制御、切断時の再接続方針を明示してください。例外は、例外であることがコードやコメントから読める必要がある。
レビュー観点チェックリスト
http.Get/http.Postを直接使っていないかhttp.Clientにtimeout方針があるか- client生成責務が外部API単位に集約されているか
contextがhttp.NewRequestWithContextで伝播しているか- リトライ込みの総待ち時間がSLAを超えないか
- timeout例外の通信では、別の停止条件が説明されているか
- timeout発生時のエラー文脈がログで追えるか
まとめ
GoのHTTP呼び出しでは、通信できることよりも、通信が返らないときに止まれることが重要である。
レビューでは、client timeout、context伝播、リトライ込みの時間予算をセットで確認したい。
timeout未設定は、正常系では見えない設計漏れである。
外部APIを呼ぶPRでは、待ち時間の責務を先に読むことで、本番障害時の滞留を防ぎやすくなる。