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

このコミットは、Go言語の標準ライブラリである net/http パッケージにおける重要なバグ修正とテストの改善を目的としています。具体的には、HTTPトランスポート層でのコネクション管理、特にレスポンスボディが完全に読み込まれる前にコネクションが閉じられてしまうというリグレッション（退行バグ）を修正しています。また、既存のテストの失敗を一時的に抑制し、新しいクライアントテストを追加することで、将来的な安定性向上に貢献しています。

コミット

commit 30c0d2315e1c6bdd7a0ba4f7c9e498486cecb750
Author: Brad Fitzpatrick <bradfitz@golang.org>
Date:   Mon May 21 10:39:31 2012 -0700

    net/http: fix regression and mute known test failure for now
    
    Two tests added in 820ffde8c are expected to fail until the fix
    for Issue 3540 goes back in (pending Windows net fixes), so
    make those tests just Logf for now, with a TODO to re-enable.
    
    Add a new client test.
    
    Rearrange the transport code to be more readable, and fix the
    bug from 820ffde8c where the persistConn was being closed before
    the body was fully ready.
    
    Fixes #3644
    Updates #1967 (not yet fixed, but should be after Issue 3540)
    
    R=golang-dev, adg
    CC=golang-dev
    https://golang.org/cl/6211069

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

https://github.com/golang/go/commit/30c0d2315e1c6bdd7a0ba4f7c9e498486cecb750

元コミット内容

上記の「コミット」セクションに記載されている内容が、このコミットの元の内容です。

変更の背景

このコミットの主な背景は、以前のコミット 820ffde8c によって導入されたリグレッションの修正です。このリグレッションは、net/http パッケージの Transport において、HTTPレスポンスのボディが完全に読み込まれる前に、基盤となるTCPコネクション（persistConn）が誤って閉じられてしまうというものでした。これにより、クライアントがレスポンスボディを完全に読み取ることができず、データが欠落したり、予期せぬエラーが発生したりする可能性がありました。

また、820ffde8c で追加された一部のテストが、Issue 3540（Windows環境でのネットワーク関連の未解決の問題）の修正が適用されるまで失敗することが予想されていたため、これらのテストを一時的に Logf（エラーではなくログとして出力）に変更し、テストスイート全体のCI/CDパイプラインをブロックしないようにする措置も取られました。

さらに、Transport コードの可読性を向上させるためのコードの再編成も行われています。

前提知識の解説

このコミットを理解するためには、以下のGo言語の net/http パッケージに関する知識が必要です。

• net/http パッケージ: Go言語でHTTPクライアントおよびサーバーを実装するための標準ライブラリです。
• http.Client: HTTPリクエストを送信し、HTTPレスポンスを受信するクライアントを表します。
• http.Transport: http.Client の基盤となるコンポーネントで、実際のHTTPリクエストの送信、コネクションの管理（永続的なコネクションの再利用など）、プロキシの処理などを担当します。
• persistConn: http.Transport 内部で使用される構造体で、単一の永続的なTCPコネクション（Keep-Aliveコネクション）を管理します。これにより、複数のHTTPリクエストで同じTCPコネクションを再利用でき、パフォーマンスが向上します。
• readLoop: persistConn に関連付けられたゴルーチン（軽量スレッド）で、基盤となるネットワークコネクションから継続的にデータを読み取り、HTTPレスポンスを解析します。
• bodyEOFSignal: net/http パッケージ内部で使用されるメカニズムで、HTTPレスポンスボディの読み取りが完了したことを readLoop に通知するために使用されます。これにより、レスポンスボディが完全に消費されるまでコネクションが閉じられないように調整されます。
• putIdleConn: persistConn をアイドルコネクションプールに戻すためのメソッドです。これにより、コネクションが再利用可能になります。
• Connection: close ヘッダー: HTTP/1.1において、クライアントまたはサーバーがコネクションを閉じることを示すヘッダーです。このヘッダーが存在する場合、レスポンスボディの読み取りが完了した後にコネクションは閉じられます。
• Issue 3540: このコミットメッセージで言及されているIssue 3540は、Goの net/http パッケージにおける「Connection header in request is ignored by the http server」という問題です。これは、クライアントが Connection: close ヘッダーを送信しても、サーバーがそれを適切に処理せず、コネクションを閉じない場合があるという問題に関連しています。この問題は、特にWindows環境でのネットワーク関連の挙動に影響を与えていたようです。
• Issue 3644: このコミットによって修正されるバグを追跡するためのIssueです。コミットメッセージから、これは persistConn がレスポンスボディの読み取り完了前に閉じられるリグレッションに関連していることがわかります。
• Issue 1967: このコミットメッセージで「Updates #1967 (not yet fixed, but should be after Issue 3540)」と記載されています。これは、Issue 3540の修正後に解決されるべき別の問題を示唆しています。具体的な内容はコミットメッセージからは不明ですが、net/http のコネクション管理に関連する問題である可能性が高いです。

技術的詳細

このコミットの技術的詳細は、主に src/pkg/net/http/transport.go の readLoop 関数における persistConn のコネクション管理ロジックの変更と、src/pkg/net/http/transport_test.go におけるテストの修正にあります。

src/pkg/net/http/transport.go の変更点:

readLoop 関数は、persistConn が基盤となるTCPコネクションからデータを読み取り、HTTPレスポンスを処理する主要なゴルーチンです。以前の実装では、レスポンスにボディがない場合（例: HEAD リクエストや Content-Length: 0 のレスポンス）、persistConn はすぐにアイドルコネクションプールに戻されていました。しかし、レスポンスボディがある場合でも、bodyEOFSignal が発火する前にコネクションが閉じられる可能性がありました。

このコミットでは、以下の重要な変更が行われています。

1. putIdleConn の呼び出しタイミングの調整:
   • 以前は、hasBody が false の場合（レスポンスボディがない場合）にのみ、pc.t.putIdleConn(pc) が if alive ブロック内で直接呼び出されていました。
   • 変更後、hasBody が true の場合は、resp.Body.(*bodyEOFSignal).fn のコールバック関数内で putIdleConn が呼び出されるようになりました。これにより、レスポンスボディが完全に読み込まれた後にのみコネクションがプールに戻されることが保証されます。
   • hasBody が false の場合（レスポンスボディがない場合）は、if alive && !hasBody という新しい条件ブロックが追加され、その中で putIdleConn が呼び出されるようになりました。これにより、ボディがないレスポンスの場合でも、コネクションが適切にプールに戻されるタイミングが明確化されました。
2. bodyEOFSignal コールバック内の alive チェック:
   • resp.Body.(*bodyEOFSignal).fn のコールバック関数内で if alive && !pc.t.putIdleConn(pc) という条件が追加されました。これは、コネクションがまだ「生きている」場合にのみ putIdleConn を試み、もしプールに戻せなかった場合は alive = false と設定してコネクションを閉じることを意味します。これにより、コネクションの状態管理がより堅牢になります。
3. waitForBodyRead の後の alive チェックの削除:
   • 以前は waitForBodyRead が nil で、かつ !alive の場合に pc.close() が呼び出されていましたが、このロジックは削除されました。代わりに、if !alive というシンプルなチェックが readLoop の最後に移動され、waitForBodyRead の完了後にコネクションが閉じられるべきかどうかを判断するようになりました。これにより、コードのフローがより明確になります。

これらの変更により、persistConn がレスポンスボディの読み取りが完了する前に閉じられるというリグレッションが修正され、HTTPコネクションの再利用がより信頼性の高いものになりました。

src/pkg/net/http/transport_test.go の変更点:

テストファイルでは、主に以下の変更が行われています。

1. testConnSet 構造体の変更:
   • 以前は map[net.Conn]bool 型の set フィールドでコネクションの状態を管理していましたが、closed map[net.Conn]bool と list []net.Conn に変更されました。
   • closed マップは各コネクションが閉じられたかどうかを追跡し、list スライスはコネクションが作成された順序を保持します。これにより、テストでのコネクションの状態の追跡と検証がより正確になります。
2. countClosed 関数の削除と check 関数の追加:
   • 以前の countClosed 関数は、閉じられたコネクションの数を返していましたが、新しい check 関数に置き換えられました。
   • check 関数は、tcs.list 内のすべてのコネクションを反復処理し、tcs.closed マップで閉じられていないコネクションがあれば t.Logf を使用してログに出力します。
   • 重要なのは、この Logf が Errorf ではなく Logf である点です。これは、コミットメッセージにあるように、Issue 3540の修正が完了するまで、これらのテストが失敗することが予想されるため、一時的にエラーとして扱わないようにするための措置です。TODO コメントで、Issue 3540が修正されたら Errorf に戻すように指示されています。
3. 新しいテストケース TestIssue3644 の追加:
   • このテストは、Connection: close ヘッダーを持つ大きなレスポンスボディを返すHTTPサーバーをセットアップし、クライアントがそのボディを完全に読み取れることを検証します。
   • もし persistConn がボディの読み取り完了前に閉じられてしまうリグレッションが再発した場合、このテストは失敗するはずです。これにより、将来的なリグレッションを防ぐための安全網が追加されました。

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

src/pkg/net/http/transport.go

--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -567,29 +567,29 @@ func (pc *persistConn) readLoop() {
 
 		hasBody := resp != nil && resp.ContentLength != 0
 		var waitForBodyRead chan bool
-		if alive {
-			if hasBody {
-				lastbody = resp.Body
-				waitForBodyRead = make(chan bool)
-				resp.Body.(*bodyEOFSignal).fn = func() {
-					if !pc.t.putIdleConn(pc) {
-						alive = false
-					}
-					waitForBodyRead <- true
-				}
-			} else {
-				// When there's no response body, we immediately
-				// reuse the TCP connection (putIdleConn), but
-				// we need to prevent ClientConn.Read from
-				// closing the Response.Body on the next
-				// loop, otherwise it might close the body
-				// before the client code has had a chance to
-				// read it (even though it'll just be 0, EOF).
-				lastbody = nil
-
-				if !pc.t.putIdleConn(pc) {
-					alive = false
-				}
-			}
+		if hasBody {
+			lastbody = resp.Body
+			waitForBodyRead = make(chan bool)
+			resp.Body.(*bodyEOFSignal).fn = func() {
+				if alive && !pc.t.putIdleConn(pc) {
+					alive = false
+				}
+				waitForBodyRead <- true
+			}
+		}
+
+		if alive && !hasBody {
+			// When there's no response body, we immediately
+			// reuse the TCP connection (putIdleConn), but
+			// we need to prevent ClientConn.Read from
+			// closing the Response.Body on the next
+			// loop, otherwise it might close the body
+			// before the client code has had a chance to
+			// read it (even though it'll just be 0, EOF).
+			lastbody = nil
+
+			if !pc.t.putIdleConn(pc) {
+				alive = false
+			}
 		}
 
 		if waitForBodyRead != nil {
-			<-waitForBodyRead
-		} else if !alive {
-			// If waitForBodyRead is nil, and we're not alive, we
-			// must close the connection before we leave the loop.
+			<-waitForBodyRead
+		}
+
+		if !alive {
 			pc.close()
 		}
 	}

src/pkg/net/http/transport_test.go

--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -48,27 +48,28 @@ func (conn *testCloseConn) Close() error {
 }
 
 type testConnSet struct {
-	set   map[net.Conn]bool
-	mutex sync.Mutex
+	closed map[net.Conn]bool
+	list   []net.Conn // in order created
+	mutex  sync.Mutex
 }
 
 func (tcs *testConnSet) insert(c net.Conn) {
 	tcs.mutex.Lock()
 	defer tcs.mutex.Unlock()
-	tcs.set[c] = true
+	tcs.closed[c] = false
+	tcs.list = append(tcs.list, c)
 }
 
 func (tcs *testConnSet) remove(c net.Conn) {
 	tcs.mutex.Lock()
 	defer tcs.mutex.Unlock()
-	// just change to false, so we have a full set of opened connections
-	tcs.set[c] = false
+	tcs.closed[c] = true
 }
 
 // some tests use this to manage raw tcp connections for later inspection
 func makeTestDial() (*testConnSet, func(n, addr string) (net.Conn, error)) {
 	connSet := &testConnSet{
-		set: make(map[net.Conn]bool),
+		closed: make(map[net.Conn]bool),
 	}
 	dial := func(n, addr string) (net.Conn, error) {
 		c, err := net.Dial(n, addr)
@@ -78,17 +79,18 @@ func makeTestDial() (*testConnSet, func(n, addr string) (net.Conn, error)) {
 	return connSet, dial
 }
 
-func (tcs *testConnSet) countClosed() (closed, total int) {
+func (tcs *testConnSet) check(t *testing.T) {
 	tcs.mutex.Lock()
 	defer tcs.mutex.Unlock()
 
-	total = len(tcs.set)
-	for _, open := range tcs.set {
-		if !open {
-			closed += 1
+	for i, c := range tcs.list {
+		if !tcs.closed[c] {
+			// TODO(bradfitz,gustavo): make the following
+			// line an Errorf, not Logf, once issue 3540
+			// is fixed again.
+			t.Logf("TCP connection #%d (of %d total) was not closed", i+1, len(tcs.list))
 		}
 	}
-	return
 }
 
 // Two subsequent requests and verify their response is the same.
@@ -175,10 +177,7 @@ func TestTransportConnectionCloseOnResponse(t *testing.T) {
 		tr.CloseIdleConnections()
 	}
 
-	closed, total := connSet.countClosed()
-	if closed < total {
-		t.Errorf("%d out of %d tcp connections were not closed", total-closed, total)
-	}
+	connSet.check(t)
 }
 
 func TestTransportConnectionCloseOnRequest(t *testing.T) {
@@ -228,10 +227,7 @@ func TestTransportConnectionCloseOnRequest(t *testing.T) {
 		tr.CloseIdleConnections()
 	}
 
-	closed, total := connSet.countClosed()
-	if closed < total {
-		t.Errorf("%d out of %d tcp connections were not closed", total-closed, total)
-	}
+	connSet.check(t)
 }
 
 func TestTransportIdleCacheKeys(t *testing.T) {
@@ -806,6 +802,35 @@ func TestTransportIdleConnCrash(t *testing.T) {
 	<-didreq
 }
 
+// Test that the transport doesn't close the TCP connection early,
+// before the response body has been read.  This was a regression
+// which sadly lacked a triggering test.  The large response body made
+// the old race easier to trigger.
+func TestIssue3644(t *testing.T) {
+	const numFoos = 5000
+	ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+		w.Header().Set("Connection", "close")
+		for i := 0; i < numFoos; i++ {
+			w.Write([]byte("foo "))
+		}
+	}))
+	defer ts.Close()
+	tr := &Transport{}
+	c := &Client{Transport: tr}
+	res, err := c.Get(ts.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	bs, err := ioutil.ReadAll(res.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(bs) != numFoos*len("foo ") {
+		t.Errorf("unexpected response length")
+	}
+}
+
 type fooProto struct{}
 
 func (fooProto) RoundTrip(req *Request) (*Response, error) {

コアとなるコードの解説

src/pkg/net/http/transport.go の readLoop 関数

この関数の変更は、persistConn がアイドルコネクションプールに戻されるタイミングを正確に制御することに焦点を当てています。

• 変更前:
  • hasBody が true の場合（レスポンスボディがある場合）、resp.Body.(*bodyEOFSignal).fn のコールバック内で putIdleConn が呼び出されていましたが、その外側の if alive ブロックの条件が複雑で、ボディの読み取りが完了する前にコネクションが閉じられる可能性がありました。
  • hasBody が false の場合（レスポンスボディがない場合）、putIdleConn はすぐに呼び出されていました。
• 変更後:
  • if hasBody ブロックが独立し、レスポンスボディがある場合の putIdleConn の呼び出しは、bodyEOFSignal のコールバック関数内に完全にカプセル化されました。これにより、ボディが完全に読み込まれるまでコネクションがプールに戻されないことが保証されます。
  • if alive && !hasBody という新しいブロックが追加され、レスポンスボディがない場合の putIdleConn の呼び出しが明確に分離されました。これにより、ボディの有無にかかわらず、コネクションの再利用ロジックがより直感的になりました。
  • bodyEOFSignal のコールバック内で if alive && !pc.t.putIdleConn(pc) という条件が追加されたことで、コネクションがまだ有効な場合にのみプールに戻す試みが行われ、失敗した場合はコネクションを閉じるという堅牢なエラーハンドリングが導入されました。
  • waitForBodyRead の後の else if !alive ブロックが削除され、if !alive が関数の最後に移動されたことで、コネクションのクローズ処理がよりシンプルかつ一貫性のあるものになりました。

これらの変更により、HTTPレスポンスボディの完全な読み取りとコネクションの再利用の間の競合状態が解消され、net/http クライアントの信頼性が向上しました。

src/pkg/net/http/transport_test.go のテストコード

テストコードの変更は、主にコネクションのクローズ状態をより正確に追跡し、リグレッションを検出するための新しいテストを追加することにあります。

• testConnSet の改善:
  • set マップから closed マップと list スライスへの変更は、コネクションのライフサイクルをより詳細に追跡できるようにするためのものです。list はコネクションの作成順序を保持し、closed は各コネクションが閉じられたかどうかをブール値で示します。これにより、テストが特定のコネクションのクローズ状態を正確に検証できるようになります。
• countClosed から check への移行:
  • countClosed は単に閉じられたコネクションの数を返していましたが、check は閉じられていないコネクションを具体的に特定し、t.Logf でログに出力します。これは、Issue 3540が未解決であるため、一時的にテスト失敗をエラーとして扱わないようにするための重要な変更です。これにより、CI/CDパイプラインがブロックされることなく、開発が継続できます。
• TestIssue3644 の追加:
  • この新しいテストは、Connection: close ヘッダーを持つ大きなレスポンスボディを返すサーバーに対してHTTPリクエストを行い、クライアントがそのボディを完全に読み取れることを確認します。これは、以前のリグレッション（ボディの読み取り完了前にコネクションが閉じられる問題）を直接的に検出するためのものです。このテストの追加により、将来的に同様のリグレッションが発生した場合に早期に発見できるようになります。

これらのテストの変更は、バグ修正の検証だけでなく、将来的なコード変更に対する安全網としても機能します。

関連リンク

• Go Issue 3644: https://github.com/golang/go/issues/3644 (このコミットによって修正されたバグのIssue)
• Go Issue 3540: https://github.com/golang/go/issues/3540 (Windows環境でのネットワーク関連の未解決の問題、このコミットでテストが一時的にミュートされた原因)
• Go Issue 1967: https://github.com/golang/go/issues/1967 (このコミットで更新されたが、まだ修正されていない問題)
• Gerrit Code Review for this commit: https://golang.org/cl/6211069

参考にした情報源リンク

• Go net/http パッケージのドキュメント: https://pkg.go.dev/net/http
• Go net パッケージのドキュメント: https://pkg.go.dev/net
• HTTP Persistent Connections (Keep-Alive): https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection
• persistConn readLoop bodyEOFSignal に関するGoの内部実装に関する情報 (Web検索結果より)
• Go Issue Tracker: https://github.com/golang/go/issues
• Goのコミット 820ffde8c に関する情報 (このコミットメッセージからの推測)