[インデックス 12924] ファイルの概要

このコミットは、Go言語の標準ライブラリである net/http/httputil パッケージ内の ReverseProxy のテストにおける不安定性（flakiness）を解消することを目的としています。具体的には、runtime.NumGoroutines() の挙動に依存していたテストロジックを改善し、より信頼性の高いテストメカニズムを導入しています。

コミット

commit a6d4471b2b38f4e865cdc4d31ae0de1e8db45a7b
Author: Colby Ranger <cranger@google.com>
Date:   Fri Apr 20 09:31:23 2012 -0700

    net/http/httputil: Made reverseproxy test less flaky.
    
    The reverseproxy test depended on the behavior of
    runtime.NumGoroutines(), which makes no guarantee when
    goroutines are reaped. Instead, modify the flushLoop()
    to invoke a callback when it returns, so the exit
    from the loop can be tested, instead of the number
    of gorountines running.
    
    R=bradfitz
    CC=golang-dev
    https://golang.org/cl/6068046

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/a6d4471b2b38f4e865cdc4d31ae0de1e8db45a7b

元コミット内容

このコミットの元の内容は、net/http/httputil パッケージの ReverseProxy のテストが不安定であるという問題に対処しています。特に、runtime.NumGoroutines() を使用してゴルーチンの数を検証する部分が問題でした。runtime.NumGoroutines() はゴルーチンがいつ回収されるかについて保証しないため、テストが時々失敗する原因となっていました。

変更の要点は以下の通りです。

• runtime.NumGoroutines() に依存するテストロジックを削除。
• flushLoop() が終了した際にコールバックを呼び出すように変更。これにより、ゴルーチンの数ではなく、ループの終了を直接テストできるようになります。

変更の背景

Go言語の net/http/httputil パッケージは、HTTPリバースプロキシを実装するためのユーティリティを提供します。このパッケージの ReverseProxy は、受信したHTTPリクエストを別のサーバーに転送し、その応答をクライアントにプロキシする機能を持っています。

このコミットが行われた背景には、ReverseProxy のテスト、特に TestReverseProxyFlushInterval が不安定（flaky）であるという問題がありました。不安定なテストとは、コードの変更がないにもかかわらず、実行するたびに成功したり失敗したりするテストのことです。このようなテストは、開発者がコードの品質を信頼することを困難にし、CI/CDパイプラインの効率を低下させます。

元のテストでは、runtime.NumGoroutines() を使用して、テスト実行前後のゴルーチンの数を比較し、ゴルーチンリークがないことを確認しようとしていました。しかし、Goのランタイムはゴルーチンがいつ正確に終了し、そのリソースがいつ解放されるかについて厳密な保証をしません。ゴルーチンのスケジューリングやガベージコレクションのタイミングは、システムの状態や他の並行処理の状況によって変動するため、runtime.NumGoroutines() の値は予測不可能になることがあります。

この不確実性が、テストが「flaky」になる主な原因でした。テストがゴルーチンの正確な数に依存していると、ゴルーチンがまだ終了していない、あるいは予期せず終了したなどの理由で、テストが誤って失敗する可能性がありました。開発者は、この不安定なテストを修正し、より堅牢で信頼性の高いテストメカニズムを導入する必要がありました。

前提知識の解説

Go言語のゴルーチン (Goroutines)

Go言語におけるゴルーチンは、軽量な並行処理の単位です。OSのスレッドよりもはるかに軽量であり、数千、数万のゴルーチンを同時に実行することが可能です。ゴルーチンはGoランタイムによって管理され、複数のOSスレッドに多重化されて実行されます。

• 軽量性: ゴルーチンは数KBのスタックサイズから始まり、必要に応じて動的にスタックを拡張・縮小します。これにより、多数のゴルーチンを効率的に起動できます。
• 並行性: go キーワードを使って関数を呼び出すことで、新しいゴルーチンが起動し、その関数が他のゴルーチンと並行して実行されます。
• スケジューリング: GoランタイムのスケジューラがゴルーチンをOSスレッドにマッピングし、実行を管理します。開発者は明示的にスレッドを管理する必要がありません。
• 通信: ゴルーチン間の通信は、主にチャネル（channels）を通じて行われます。チャネルは、ゴルーチン間で値を安全に送受信するためのメカニズムであり、共有メモリによる競合状態を避けるためのGoのイディオムです。

runtime.NumGoroutines()

runtime.NumGoroutines() 関数は、現在実行中のゴルーチンの数を返します。この関数はデバッグやプロファイリングの目的で役立ちますが、テストにおいてゴルーチンのライフサイクルを厳密に検証するために使用すると、不安定なテスト（flaky test）の原因となることがあります。

その理由は以下の通りです。

• 非同期性: ゴルーチンの起動や終了は非同期に行われます。runtime.NumGoroutines() を呼び出した瞬間に、すべてのゴルーチンが期待通りの状態にあるとは限りません。
• ガベージコレクションとスケジューリング: Goランタイムは、終了したゴルーチンのリソースをいつ解放するかについて、厳密な保証をしません。また、ゴルーチンのスケジューリングも動的であり、特定のゴルーチンがいつ実行され、いつブロックされるかは予測が難しい場合があります。
• テストのタイミング問題: テストコードが runtime.NumGoroutines() を呼び出すタイミングと、対象のゴルーチンが実際に終了するタイミングとの間に競合状態が生じることがあります。これにより、テストが期待するゴルーチン数と実際の数が一致せず、テストが失敗することがあります。

不安定なテスト (Flaky Tests)

不安定なテストとは、同じコードベースに対して同じテストを複数回実行したときに、結果が成功と失敗の間で変動するテストのことです。これは、テストが外部要因（ネットワークの遅延、データベースの状態、システム時刻など）や、並行処理におけるタイミングの問題（競合状態、デッドロックなど）に依存している場合に発生しやすいです。

不安定なテストは、以下のような問題を引き起こします。

• 信頼性の低下: 開発者はテスト結果を信頼できなくなり、テストが失敗しても「また不安定なテストか」と無視するようになる可能性があります。
• 開発効率の低下: 不安定なテストがCI/CDパイプラインで失敗すると、開発者はその原因を調査するために時間を費やすことになりますが、実際にはコードに問題がない場合もあります。
• バグの見逃し: 本当のバグが不安定なテストの失敗に紛れて見過ごされる可能性があります。

このコミットは、まさに runtime.NumGoroutines() の非同期性によって引き起こされる不安定なテストの問題を解決しようとしています。

技術的詳細

このコミットの技術的な核心は、テストの信頼性を向上させるために、ゴルーチンの数を直接監視するのではなく、特定のゴルーチン（flushLoop()）の終了をイベントベースで検出するメカニズムに切り替えた点にあります。

元の TestReverseProxyFlushInterval テストでは、runtime.NumGoroutines() を使用して、テスト開始時と終了時のゴルーチン数の差分をチェックしていました。これは、ReverseProxy が内部的に起動する flushLoop() ゴルーチンが適切に終了していることを確認するためのものでした。しかし、前述の通り、runtime.NumGoroutines() はゴルーチンのライフサイクルに関する厳密な保証を提供しないため、テストが不安定になっていました。

新しいアプローチでは、以下の変更が導入されました。

1. onExitFlushLoop コールバックの導入: src/pkg/net/http/httputil/reverseproxy.go に、グローバル変数 onExitFlushLoop func() が追加されました。この変数は、テストから設定されるコールバック関数を保持します。 flushLoop() ゴルーチンが select ステートメント内の m.done チャネルからのシグナルを受け取って終了する直前に、この onExitFlushLoop が nil でなければ呼び出されるように変更されました。

   // src/pkg/net/http/httputil/reverseproxy.go
   // ...
   // onExitFlushLoop is a callback set by tests to detect the state of the
   // flushLoop() goroutine.
   var onExitFlushLoop func()
   // ...
   func (m *maxLatencyWriter) flushLoop() {
   	for {
   		select {
   		case <-m.done:
   			if onExitFlushLoop != nil {
   				onExitFlushLoop() // flushLoopが終了する直前にコールバックを呼び出す
   			}
   			return
   		// ...
   		}
   	}
   }
   

2. テストロジックの変更: src/pkg/net/http/httputil/reverseproxy_test.go の TestReverseProxyFlushInterval テストが大幅に修正されました。

   • runtime.NumGoroutines() を使用したゴルーチン数のチェックが完全に削除されました。
   • beforeCopyResponse という以前のテスト用コールバックも削除されました。
   • 代わりに、done := make(chan bool) というチャネルが作成され、onExitFlushLoop コールバックがこの done チャネルに true を送信するように設定されました。
   • テストの最後に、select ステートメントを使用して done チャネルからの受信を待ちます。これにより、flushLoop() が実際に終了したことを直接確認できます。タイムアウト（5秒）も設定されており、指定時間内に終了しない場合はテストが失敗します。

   // src/pkg/net/http/httputil/reverseproxy_test.go
   // ...
   func TestReverseProxyFlushInterval(t *testing.T) {
   	// ...
   	done := make(chan bool)
   	onExitFlushLoop = func() { done <- true } // flushLoop終了時にdoneチャネルにシグナルを送る
   	defer func() { onExitFlushLoop = nil }()
   	// ...
   	// HTTPリクエストの実行
   	// ...
   	select {
   	case <-done: // flushLoopが終了したことを確認
   		// OK
   	case <-time.After(5 * time.Second): // 5秒以内に終了しない場合はエラー
   		t.Error("maxLatencyWriter flushLoop() never exited")
   	}
   }
   

この変更により、テストはゴルーチンの内部的なスケジューリングやガベージコレクションのタイミングに依存することなく、flushLoop() の論理的な終了イベントを直接捕捉できるようになりました。これにより、テストの信頼性が大幅に向上し、不安定な失敗が解消されます。これは、並行処理を含むシステムのテストにおいて、イベントベースのアプローチがゴルーチン数の直接的な監視よりも優れていることを示す良い例です。

コアとなるコードの変更箇所

src/pkg/net/http/httputil/reverseproxy.go

--- a/src/pkg/net/http/httputil/reverseproxy.go
+++ b/src/pkg/net/http/httputil/reverseproxy.go
@@ -17,9 +17,9 @@ import (
 	"time"
 )
 
-// beforeCopyResponse is a callback set by tests to intercept the state of the
-// output io.Writer before the data is copied to it.
-var beforeCopyResponse func(dst io.Writer)
+// onExitFlushLoop is a callback set by tests to detect the state of the
+// flushLoop() goroutine.
+var onExitFlushLoop func()
 
 // ReverseProxy is an HTTP Handler that takes an incoming request and
 // sends it to another server, proxying the response back to the
@@ -138,9 +138,6 @@ func (p *ReverseProxy) copyResponse(dst io.Writer, src io.Reader) {
 		}
 	}
 
-	if beforeCopyResponse != nil {
-		beforeCopyResponse(dst)
-	}
 	io.Copy(dst, src)
 }
 
@@ -169,6 +166,9 @@ func (m *maxLatencyWriter) flushLoop() {
 	for {
 		select {
 		case <-m.done:
+			if onExitFlushLoop != nil {
+				onExitFlushLoop()
+			}
 			return
 		case <-t.C:
 			m.lk.Lock()

src/pkg/net/http/httputil/reverseproxy_test.go

--- a/src/pkg/net/http/httputil/reverseproxy_test.go
+++ b/src/pkg/net/http/httputil/reverseproxy_test.go
@@ -7,12 +7,10 @@
 package httputil
 
 import (
-	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"runtime"
 	"testing"
 	"time"
 )
@@ -112,10 +110,6 @@ func TestReverseProxyQuery(t *testing.T) {
 }
 
 func TestReverseProxyFlushInterval(t *testing.T) {
-	if testing.Short() {
-		return
-	}
-
 	const expected = "hi"
 	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Write([]byte(expected))
@@ -130,38 +124,28 @@ func TestReverseProxyFlushInterval(t *testing.T) {
 	proxyHandler := NewSingleHostReverseProxy(backendURL)
 	proxyHandler.FlushInterval = time.Microsecond
 
-	dstChan := make(chan io.Writer, 1)
-	beforeCopyResponse = func(dst io.Writer) { dstChan <- dst }
-	defer func() { beforeCopyResponse = nil }()
+	done := make(chan bool)
+	onExitFlushLoop = func() { done <- true }
+	defer func() { onExitFlushLoop = nil }()
 
 	frontend := httptest.NewServer(proxyHandler)
 	defer frontend.Close()
 
-	initGoroutines := runtime.NumGoroutine()
-	for i := 0; i < 100; i++ {
-		req, _ := http.NewRequest("GET", frontend.URL, nil)
-		req.Close = true
-		res, err := http.DefaultClient.Do(req)
-		if err != nil {
-			t.Fatalf("Get: %v", err)
-		}
-		if bodyBytes, _ := ioutil.ReadAll(res.Body); string(bodyBytes) != expected {
-			t.Errorf("got body %q; expected %q", bodyBytes, expected)
-		}
-
-		select {
-		case dst := <-dstChan:
-			if _, ok := dst.(*maxLatencyWriter); !ok {
-				t.Errorf("got writer %T; expected %T", dst, &maxLatencyWriter{})
-			}
-		default:
-			t.Error("maxLatencyWriter Write() was never called")
-		}
-
-		res.Body.Close()
+	req, _ := http.NewRequest("GET", frontend.URL, nil)
+	req.Close = true
+	res, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
 	}
-	// Allow up to 50 additional goroutines over 100 requests.
-	if delta := runtime.NumGoroutine() - initGoroutines; delta > 50 {
-		t.Errorf("grew %d goroutines; leak? 環境", delta)
+	defer res.Body.Close()
+	if bodyBytes, _ := ioutil.ReadAll(res.Body); string(bodyBytes) != expected {
+		t.Errorf("got body %q; expected %q", bodyBytes, expected)
+	}
+
+	select {
+	case <-done:
+		// OK
+	case <-time.After(5 * time.Second):
+		t.Error("maxLatencyWriter flushLoop() never exited")
 	}
 }

コアとなるコードの解説

reverseproxy.go の変更点

1. beforeCopyResponse の削除と onExitFlushLoop の追加:

   • beforeCopyResponse は、copyResponse 関数がデータをコピーする前に呼び出されるテスト用のコールバックでした。これは今回の不安定性の原因とは直接関係なく、テストロジックの変更に伴い不要になったため削除されました。
   • 新しく追加された onExitFlushLoop は func() 型のグローバル変数です。これは flushLoop() ゴルーチンが終了する際に呼び出されるコールバックとして機能します。テストはこのコールバックを設定することで、flushLoop() の終了イベントを捕捉できるようになります。

2. flushLoop() 内での onExitFlushLoop の呼び出し:

   • maxLatencyWriter の flushLoop() メソッドは、select ステートメント内で m.done チャネルからのシグナルを待機しています。このシグナルは、flushLoop() がその役割を終えて終了すべきであることを示します。
   • m.done からシグナルを受け取った直後、return する前に、onExitFlushLoop が nil でない場合にその関数が呼び出されるように変更されました。これにより、テストは flushLoop() が正常に終了したことを正確に検知できます。

reverseproxy_test.go の変更点

1. runtime パッケージのインポート削除:

   • runtime.NumGoroutines() が使用されなくなったため、runtime パッケージのインポートが削除されました。

2. TestReverseProxyFlushInterval の大幅な修正:

   • testing.Short() の削除: testing.Short() は、テストが長時間かかる場合にスキップするためのGoの慣習ですが、このテストの不安定性とは直接関係なく、テストロジックの簡素化の一環として削除された可能性があります。
   • dstChan と beforeCopyResponse 関連ロジックの削除: beforeCopyResponse コールバックが削除されたため、それに関連するチャネル (dstChan) やロジックも削除されました。
   • runtime.NumGoroutines() を使用したゴルーチンリークチェックの削除: 最も重要な変更点です。不安定性の原因となっていた initGoroutines と runtime.NumGoroutines() を使用したゴルーチン数の比較ロジックが完全に削除されました。
   • done チャネルと onExitFlushLoop の設定:
     • done := make(chan bool) という新しいチャネルが作成されました。これは、flushLoop() が終了したことをテストに通知するためのものです。
     • onExitFlushLoop グローバル変数に、この done チャネルに true を送信する匿名関数が設定されます。defer を使用して、テスト終了時に onExitFlushLoop が nil にリセットされるようにしています。
   • 単一のリクエストに簡素化: 元のテストでは100回のリクエストをループしていましたが、新しいテストでは単一のリクエストに簡素化されました。これは、ゴルーチンリークのチェックがイベントベースになったため、多数のリクエストを繰り返す必要がなくなったためと考えられます。
   • select を用いた flushLoop 終了の待機:
     • テストの最後に select ステートメントが導入されました。
     • case <-done: は、flushLoop() が onExitFlushLoop コールバックを呼び出し、done チャネルにシグナルを送信するのを待ちます。シグナルを受信すれば、flushLoop() が正常に終了したと判断できます。
     • case <-time.After(5 * time.Second): はタイムアウトを設定しています。もし5秒以内に flushLoop() が終了しない場合、テストはエラーとなり、flushLoop() がハングしている可能性を示します。

これらの変更により、テストはゴルーチンの内部的な挙動に依存するのではなく、flushLoop() の明確な終了イベントを待機するようになりました。これにより、テストの実行がより予測可能になり、不安定な失敗が解消されます。これは、並行処理のテストにおいて、タイミングに依存するアサーションを避け、イベント駆動型のアプローチを採用することの重要性を示しています。

関連リンク

• Go言語の並行処理 (Goroutines and Channels): https://go.dev/tour/concurrency/1
• net/http/httputil パッケージのドキュメント: https://pkg.go.dev/net/http/httputil
• Go言語のテスト: https://go.dev/doc/tutorial/add-a-test

参考にした情報源リンク

• Go CL 6068046: net/http/httputil: Made reverseproxy test less flaky.: https://golang.org/cl/6068046
• Go言語の runtime パッケージ: https://pkg.go.dev/runtime
• Flaky Tests (不安定なテスト) について: (一般的なソフトウェアテストの概念であり、特定のGoのドキュメントではないが、関連情報として)
  • Martin Fowler - Flaky Test: https://martinfowler.com/bliki/FlakyTest.html