Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

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

このコミットは、Go言語の標準ライブラリである net/smtp パッケージ内のテストファイル smtp_test.go に存在するデータ競合（data race）を解消することを目的としています。具体的には、TestSendMail 関数内で発生していたデータ競合を、Goのレース検出器（race detector）によって特定し、sync.WaitGroup のような同期プリミティブの代わりにチャネル (chan struct{}) を用いた適切な同期メカニズムを導入することで修正しています。これにより、テストの信頼性と正確性が向上し、並行処理における潜在的なバグが排除されました。

コミット

  • コミットハッシュ: bcb495b39aebd55dd3e4502807646eec7023c5f3
  • 作者: Rick Arnold rickarnoldjr@gmail.com
  • コミット日時: Mon Dec 17 10:45:33 2012 -0500

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

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

元コミット内容

net/smtp: remove data race from TestSendMail.

A data race was found in TestSendMail by the race detector.

Fixes #4559.

R=golang-dev, bradfitz, dave, rsc
CC=golang-dev
https://golang.org/cl/6944057

変更の背景

このコミットの主な背景は、Go言語の並行処理における一般的な問題である「データ競合（data race）」が net/smtp パッケージのテストコード TestSendMail で検出されたことです。データ競合は、複数のゴルーチン（goroutine）が同時に共有メモリにアクセスし、少なくとも1つのアクセスが書き込みである場合に発生する未定義の動作です。このような競合は、テストの実行結果を不安定にしたり、誤った結果を導いたりする可能性があり、実際のアプリケーションコードに同様の脆弱性が存在する場合には深刻なバグにつながる可能性があります。

Go 1.1から導入されたレース検出器は、このような並行処理のバグを特定するための強力なツールです。このツールが TestSendMail 内でデータ競合を報告したため、テストの正確性と信頼性を確保するために修正が必要となりました。コミットメッセージにある Fixes #4559 は、このコミットがGoのIssueトラッカーで報告された問題4559を解決することを示しています。このIssueは、TestSendMail が bcmdbuf という共有バッファに対して複数のゴルーチンから同時に書き込みを行おうとしていたために発生していました。

前提知識の解説

このコミットを理解するためには、以下のGo言語の概念と標準ライブラリの知識が役立ちます。

  • データ競合 (Data Race): 複数のゴルーチンが同時に同じメモリ位置にアクセスし、そのうち少なくとも1つが書き込み操作である場合に発生する競合状態です。データ競合は予測不能な結果を引き起こし、デバッグが困難なバグの温床となります。Goのメモリモデルでは、データ競合が発生した場合の動作は未定義とされています。
  • Goレース検出器 (Go Race Detector): Go 1.1で導入されたツールで、プログラム実行中にデータ競合を検出します。go run -race や go test -race のように -race フラグを付けて実行することで有効になります。検出された場合、競合が発生した場所とゴルーチンのスタックトレースが報告されます。
  • ゴルーチン (Goroutine): Go言語における軽量な実行スレッドです。go キーワードを使って関数呼び出しの前に置くことで、その関数を新しいゴルーチンとして並行して実行できます。
  • チャネル (Channel): ゴルーチン間で値を送受信するためのGoの同期プリミティブです。チャネルは、ゴルーチン間の安全な通信を可能にし、データ競合を防ぐための主要なメカニズムです。このコミットでは、chan struct{} 型のチャネルがゴルーチンの完了を通知するために使用されています。struct{} はゼロバイトの型であり、チャネルを通じてデータを送信する必要がない場合にメモリ効率が良いです。
  • net/smtp パッケージ: Go言語でSMTP（Simple Mail Transfer Protocol）クライアントを実装するための標準ライブラリパッケージです。メールの送信機能を提供します。
  • textproto パッケージ: テキストベースのネットワークプロトコル（HTTP、SMTP、NNTPなど）を扱うためのヘルパー関数を提供するパッケージです。textproto.NewConn は、net.Conn インターフェースをラップして、プロトコル固有の読み書き操作（例: PrintfLine, ReadLine）を容易にします。
  • bufio パッケージ: バッファリングされたI/O操作を提供するパッケージです。bufio.NewReader と bufio.NewWriter は、それぞれ io.Reader と io.Writer をラップして、効率的な読み書きを可能にします。
  • bytes.Buffer: 可変長のバイトバッファを実装する型です。io.Writer インターフェースを満たしており、バイト列を効率的に構築するために使用されます。このコミットでは、SMTPコマンドの出力をキャプチャするために使用されています。
  • strings.Split と strings.Join: strings.Split は文字列を指定された区切り文字で分割し、文字列のスライスを返します。strings.Join は文字列のスライスを指定された区切り文字で結合し、単一の文字列を返します。このコミットでは、改行コードの正規化（\n を \r\n に変換）のために使用されています。
  • net.Listener: ネットワーク接続をリッスンするためのインターフェースです。l.Accept() メソッドは、新しい接続が確立されるまでブロックし、その接続を表す net.Conn を返します。
  • defer ステートメント: 関数がリターンする直前に実行される関数呼び出しをスケジュールします。defer conn.Close() は、接続が閉じられることを保証するために使用されます。
  • t.Fatalf() / t.Errorf() / t.Log(): Goのテストパッケージ testing で提供される関数です。
    • t.Fatalf(format, args...): テストを失敗としてマークし、メッセージをログに出力して、現在のテストゴルーチンを停止します。
    • t.Errorf(format, args...): テストを失敗としてマークし、メッセージをログに出力しますが、テストゴルーチンは停止しません。
    • t.Log(args...): テスト実行中にメッセージをログに出力します。

技術的詳細

このコミットの技術的な核心は、TestSendMail 関数内で発生していたデータ競合の解消です。元のコードでは、テスト用のSMTPサーバーを模倣するゴルーチンが bcmdbuf という bytes.Buffer に書き込みを行っていました。同時に、メインのテストゴルーチンも bcmdbuf の内容を読み取ろうとしていました。bytes.Buffer は複数のゴルーチンからの同時書き込みに対して安全ではありません（読み取りと書き込みの同時アクセスも同様）。これがレース検出器によってデータ競合として報告された原因です。

修正は以下の2つの主要な変更によって行われました。

  1. basicServer / basicClient などのローカル変数化: TestBasic, TestNewClient, TestNewClient2 関数内で、グローバル変数として定義されていた basicServer, basicClient などを、それぞれのテスト関数内でローカル変数 server と client に変更しました。これにより、これらの文字列がテスト間で共有されることによる潜在的な副作用や意図しない変更を防ぎ、テストの独立性を高めています。これは直接的なデータ競合の修正ではありませんが、テストの健全性を向上させる変更です。

  2. TestSendMail における同期メカニズムの導入: これがデータ競合の直接的な修正です。

    • done チャネルの導入: var done = make(chan struct{}) という行が追加されました。これは、テスト用のSMTPサーバーを模倣するゴルーチンがその処理を完了したことをメインのテストゴルーチンに通知するためのチャネルです。struct{} はゼロバイトの型であるため、チャネルを通じて実際のデータを送信する必要がなく、シグナル通知に最適です。
    • defer close(done): サーバーゴルーチンの冒頭に defer close(done) が追加されました。これにより、サーバーゴルーチンが正常に終了するか、エラーで終了するかにかかわらず、必ず done チャネルが閉じられます。チャネルが閉じられると、そのチャネルからの読み取り操作はブロックされなくなり、値が送信されなくてもゼロ値が即座に返されます。
    • bcmdbuf.Write への変更: サーバーゴルーチン内で、クライアントからのメッセージを bcmdbuf に書き込む部分が w.Write([]byte(msg + "\\r\\n")) から bcmdbuf.Write([]byte(msg + "\\r\\n")) に変更されました。これは、w が *bufio.Writer であり、その基盤となる bytes.Buffer (cmdbuf) への書き込みが直接行われるようにするためです。これにより、bcmdbuf が共有リソースであることが明確になります。
    • <-done による待機: メインのテストゴルーチンで、SendMail の呼び出し後に <-done が追加されました。これは、サーバーゴルーチンが done チャネルを閉じるまでメインゴルーチンをブロックします。これにより、サーバーゴルーチンが bcmdbuf への書き込みを完了するまで、メインゴルーチンが bcmdbuf の内容を読み取ろうとすることがなくなり、データ競合が解消されます。

これらの変更により、TestSendMail は、テスト用のSMTPサーバーゴルーチンが bcmdbuf への書き込みを完全に終えてから、メインのテストゴルーチンがその内容を読み取るという順序が保証されるようになりました。これにより、共有リソースである bcmdbuf への同時アクセスが排除され、データ競合が解消されました。

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

変更は src/pkg/net/smtp/smtp_test.go ファイルに集中しています。

--- a/src/pkg/net/smtp/smtp_test.go
+++ b/src/pkg/net/smtp/smtp_test.go
@@ -69,13 +69,13 @@ func (f faker) SetReadDeadline(time.Time) error  { return nil }\n func (f faker) SetWriteDeadline(time.Time) error { return nil }\n \n func TestBasic(t *testing.T) {\n-\tbasicServer = strings.Join(strings.Split(basicServer, "\\n"), "\\r\\n")
-\tbasicClient = strings.Join(strings.Split(basicClient, "\\n"), "\\r\\n")
+\tserver := strings.Join(strings.Split(basicServer, "\\n"), "\\r\\n")
+\tclient := strings.Join(strings.Split(basicClient, "\\n"), "\\r\\n")
 \n \tvar cmdbuf bytes.Buffer
 \tbcmdbuf := bufio.NewWriter(&cmdbuf)
 \tvar fake faker
-\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(basicServer)), bcmdbuf)
+\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(server)), bcmdbuf)
 \tc := &Client{Text: textproto.NewConn(fake), localName: "localhost"}
 \n \tif err := c.helo(); err != nil {\n@@ -144,8 +144,8 @@ Goodbye.`
 \n \tbcmdbuf.Flush()\n \tactualcmds := cmdbuf.String()\n-\tif basicClient != actualcmds {\n-\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, basicClient)
+\tif client != actualcmds {\n+\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, client)
 \t}\n }\n \n@@ -188,8 +188,8 @@ QUIT
 `\n \n func TestNewClient(t *testing.T) {\n-\tnewClientServer = strings.Join(strings.Split(newClientServer, "\\n"), "\\r\\n")
-\tnewClientClient = strings.Join(strings.Split(newClientClient, "\\n"), "\\r\\n")
+\tserver := strings.Join(strings.Split(newClientServer, "\\n"), "\\r\\n")
+\tclient := strings.Join(strings.Split(newClientClient, "\\n"), "\\r\\n")
 \n \tvar cmdbuf bytes.Buffer
 \tbcmdbuf := bufio.NewWriter(&cmdbuf)
 \tvar out = func() string {
 \t\treturn cmdbuf.String()\n \t}\n \tvar fake faker
-\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(newClientServer)), bcmdbuf)
+\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(server)), bcmdbuf)
 \tc, err := NewClient(fake, "fake.host")
 \tif err != nil {\n \t\tt.Fatalf("NewClient: %v\\n(after %v)", err, out())
@@ -214,8 +214,8 @@ func TestNewClient(t *testing.T) {\n \t}\n \n \tactualcmds := out()\n-\tif newClientClient != actualcmds {\n-\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, newClientClient)
+\tif client != actualcmds {\n+\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, client)
 \t}\n }\n \n@@ -232,13 +232,13 @@ QUIT
 `\n \n func TestNewClient2(t *testing.T) {\n-\tnewClient2Server = strings.Join(strings.Split(newClient2Server, "\\n"), "\\r\\n")
-\tnewClient2Client = strings.Join(strings.Split(newClient2Client, "\\n"), "\\r\\n")
+\tserver := strings.Join(strings.Split(newClient2Server, "\\n"), "\\r\\n")
+\tclient := strings.Join(strings.Split(newClient2Client, "\\n"), "\\r\\n")
 \n \tvar cmdbuf bytes.Buffer
 \tbcmdbuf := bufio.NewWriter(&cmdbuf)
 \tvar fake faker
-\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(newClient2Server)), bcmdbuf)
+\tfake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(server)), bcmdbuf)
 \tc, err := NewClient(fake, "fake.host")
 \tif err != nil {\n \t\tt.Fatalf("NewClient: %v", err)
@@ -252,8 +252,8 @@ func TestNewClient2(t *testing.T) {\n \n \tbcmdbuf.Flush()\n \tactualcmds := cmdbuf.String()\n-\tif newClient2Client != actualcmds {\n-\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, newClient2Client)
+\tif client != actualcmds {\n+\t\tt.Fatalf("Got:\\n%s\\nExpected:\\n%s", actualcmds, client)
 \t}\n }\n \n@@ -385,38 +385,44 @@ func TestSendMail(t *testing.T) {\n \t}\n \tdefer l.Close()\n \n-\tgo func(l net.Listener, data []string, w *bufio.Writer) {\n-\t\ti := 0
+\t// prevent data race on bcmdbuf
+\tvar done = make(chan struct{})\n+\tgo func(data []string) {\n+\n+\t\tdefer close(done)\n+\n \t\tconn, err := l.Accept()\n \t\tif err != nil {\n-\t\t\tt.Log("Accept error: %v", err)
+\t\t\tt.Errorf("Accept error: %v", err)
 \t\t\treturn
 \t\t}\n \t\tdefer conn.Close()\n \n \t\ttc := textproto.NewConn(conn)\n-\t\tfor i < len(data) && data[i] != "" {\n+\t\tfor i := 0; i < len(data) && data[i] != ""; i++ {\n \t\t\ttc.PrintfLine(data[i])
 \t\t\tfor len(data[i]) >= 4 && data[i][3] == '-' {\n \t\t\t\ti++
 \t\t\t\ttc.PrintfLine(data[i])
 \t\t\t}\n+\t\t\tif data[i] == "221 Goodbye" {\n+\t\t\t\treturn
+\t\t\t}\n \t\t\tread := false
 \t\t\tfor !read || data[i] == "354 Go ahead" {\n \t\t\t\tmsg, err := tc.ReadLine()\n-\t\t\t\tw.Write([]byte(msg + "\\r\\n"))
+\t\t\t\t\tbcmdbuf.Write([]byte(msg + "\\r\\n"))
 \t\t\t\tread = true
 \t\t\t\tif err != nil {\n-\t\t\t\t\tt.Log("Read error: %v", err)
+\t\t\t\t\tt.Errorf("Read error: %v", err)
 \t\t\t\t\treturn
 \t\t\t\t}\n \t\t\t\tif data[i] == "354 Go ahead" && msg == "." {\n \t\t\t\t\tbreak
 \t\t\t\t}\n \t\t\t}\n-\t\t\ti++
 \t\t}\n-\t}(l, strings.Split(server, "\\r\\n"), bcmdbuf)
+\t}(strings.Split(server, "\\r\\n"))
 \n \terr = SendMail(l.Addr().String(), nil, "test@example.com", []string{"other@example.com"}, []byte(strings.Replace(`From: test@example.com\n To: other@example.com\n
@@ -429,6 +435,7 @@ SendMail is working for me.\n \t\tt.Errorf("%v", err)\n \t}\n \n+\t<-done\n \tbcmdbuf.Flush()\n \tactualcmds := cmdbuf.String()\n \tif client != actualcmds {\n```

## コアとなるコードの解説

### `TestBasic`, `TestNewClient`, `TestNewClient2` の変更

これらのテスト関数では、`basicServer`, `basicClient` などのグローバル変数への参照が、それぞれローカル変数 `server` と `client` に置き換えられました。

```go
-	basicServer = strings.Join(strings.Split(basicServer, "\n"), "\r\n")
-	basicClient = strings.Join(strings.Split(basicClient, "\n"), "\r\n")
+	server := strings.Join(strings.Split(basicServer, "\n"), "\r\n")
+	client := strings.Join(strings.Split(basicClient, "\n"), "\r\n")

これは、テストの独立性を高め、意図しない副作用を防ぐためのリファクタリングです。各テストが独自のサーバー/クライアント文字列のコピーを持つことで、並行して実行された場合でも互いに干渉しないようになります。

TestSendMail の変更

この関数がデータ競合の修正の中心です。

  1. done チャネルの導入:

    	// prevent data race on bcmdbuf
	var done = make(chan struct{})

    bcmdbuf 上でのデータ競合を防ぐために、done という名前のチャネルが作成されました。このチャネルは、サーバーを模倣するゴルーチンがその処理を完了したことをメインのテストゴルーチンに通知するために使用されます。

  2. サーバーゴルーチンのシグネチャ変更と defer close(done):

    -	go func(l net.Listener, data []string, w *bufio.Writer) {
+	go func(data []string) {
+
+		defer close(done)

    サーバーゴルーチンの引数から l net.Listener と w *bufio.Writer が削除され、data []string のみが渡されるようになりました。l はゴルーチン内で l.Accept() で取得されるため不要になり、w は後述の変更で直接 bcmdbuf を使用するため不要になりました。 最も重要な変更は defer close(done) の追加です。これにより、このゴルーチンが終了する際に必ず done チャネルが閉じられます。

  3. bcmdbuf.Write への変更:

    -			w.Write([]byte(msg + "\r\n"))
+				bcmdbuf.Write([]byte(msg + "\r\n"))

    サーバーゴルーチン内で、クライアントからのメッセージをバッファに書き込む部分が、引数として渡されていた w *bufio.Writer を介するのではなく、直接 bcmdbuf に書き込むように変更されました。これにより、bcmdbuf が共有リソースであることが明確になり、そのアクセスが done チャネルによって同期される対象となります。

  4. サーバーゴルーチンへの引数変更:

    -	}(l, strings.Split(server, "\r\n"), bcmdbuf)
+	}(strings.Split(server, "\r\n"))

    サーバーゴルーチンを起動する際の引数も変更されました。l と bcmdbuf はゴルーチン内で直接アクセスされるため、引数として渡す必要がなくなりました。

  5. メインゴルーチンでの待機:

    	<-done
	bcmdbuf.Flush()

    SendMail の呼び出し後、メインのテストゴルーチンは <-done でブロックされます。これは、サーバーゴルーチンが done チャネルを閉じるまで、メインゴルーチンが次の処理（bcmdbuf.Flush() や cmdbuf.String() による読み取り）に進まないことを保証します。これにより、サーバーゴルーチンが bcmdbuf への書き込みを完了する前にメインゴルーチンが読み取りを行うというデータ競合が完全に解消されます。

これらの変更により、TestSendMail は並行処理において安全になり、レース検出器によって報告されていた問題が解決されました。

関連リンク

参考にした情報源リンク