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

このコミットは、Go言語の標準ライブラリである database/sql パッケージにおける複数の重要なバグ修正と、接続リークを検出するためのテストの追加を目的としています。具体的には、データベース接続のオープン数を誤って二重にデクリメントしてしまう問題、Row.Scan() メソッドが *RawBytes の不適切な使用時に Rows 構造体をリークする問題、そして SetMaxIdleConns 関数における潜在的なデッドロック/競合状態を修正しています。さらに、テストスイートの終了時に開いている接続が残っていないことを確認するチェックが追加され、将来的な接続リークの検出能力が向上しています。

コミット

commit 478f4b67543824c039d2f7afec6af88a59148db2
Author: Alberto García Hierro <alberto@garciahierro.com>
Date:   Wed Oct 16 09:17:25 2013 -0700

    database/sql: fix double decrement of numOpen count; test for connection leaks
    
    Add a check at the end of every test to make sure
    there are no leaked connections after running a test.
    
    Avoid incorrectly decrementing the number of open connections
    when the driver connection ends up it a bad state (numOpen was
    decremented twice).
    
    Prevent leaking a Rows struct (which ends up leaking a
    connection) in Row.Scan() when a *RawBytes destination is
    improperly used.
    
    Close the Rows struct in TestRowsColumns.
    
    Update #6593
    
    R=golang-dev, bradfitz, dave
    CC=golang-dev
    https://golang.org/cl/14642044

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

https://github.com/golang/go/commit/478f4b67543824c039d2f7afec6af88a59148db2

元コミット内容

database/sql: fix double decrement of numOpen count; test for connection leaks
    
Add a check at the end of every test to make sure
there are no leaked connections after running a test.

Avoid incorrectly decrementing the number of open connections
when the driver connection ends up it a bad state (numOpen was
decremented twice).

Prevent leaking a Rows struct (which ends up leaking a
connection) in Row.Scan() when a *RawBytes destination is
improperly used.

Close the Rows struct in TestRowsColumns.

Update #6593

変更の背景

このコミットは、database/sql パッケージにおける複数の深刻なバグと、テストカバレッジの不足に対処するために行われました。

1. numOpen カウントの二重デクリメント: データベース接続が不良状態（driver.ErrBadConn）になった際に、オープン接続数を管理する numOpen カウンタが誤って二重にデクリメントされる問題がありました。これにより、database/sql が認識するオープン接続の数が実際の数よりも少なくなり、接続プールの管理が破綻し、最終的には利用可能な接続が枯渇するなどの問題を引き起こす可能性がありました。
2. Row.Scan() における Rows 構造体のリーク: Row.Scan() メソッドが *RawBytes 型の引数を不適切に受け取った場合にエラーを返すことがありますが、この際に Rows 構造体が適切にクローズされず、結果として基盤となるデータベース接続がリークする問題がありました。接続リークは、アプリケーションの実行中に利用可能なデータベース接続が徐々に失われ、最終的にデータベース操作ができなくなる原因となります。
3. SetMaxIdleConns における潜在的なデッドロック/競合状態: SetMaxIdleConns 関数内でアイドル接続をクローズする際に、db.mu ミューテックスを保持したまま dc.Close() を呼び出していました。dc.Close() 内部でも同じミューテックスが取得される可能性があるため、デッドロックや競合状態が発生するリスクがありました。
4. 接続リーク検出の不足: 既存のテストスイートでは、テスト実行後にデータベース接続が適切にクローズされているか、リークが発生していないかを自動的に検証する仕組みがありませんでした。これにより、上記のような接続リークバグが発見されにくくなっていました。

これらの問題は、データベース接続の安定性とリソース管理に直接影響を与えるため、修正が急務でした。

前提知識の解説

database/sql パッケージ

Go言語の database/sql パッケージは、SQLデータベースへの汎用的なインターフェースを提供します。これは特定のデータベースドライバに依存しない抽象化レイヤーであり、アプリケーションコードは database/sql のAPIを使用してデータベースと対話します。実際のデータベースとの通信は、database/sql/driver インターフェースを実装した各データベースドライバ（例: github.com/go-sql-driver/mysql や github.com/lib/pq）によって行われます。

主要な概念:

• DB 構造体: データベースへの接続プールを表します。アプリケーションは通常、この DB オブジェクトを介してデータベース操作を行います。
• 接続プール: database/sql は、データベースへの接続を効率的に再利用するために接続プールを管理します。これにより、新しい接続を確立するオーバーヘッドを削減し、パフォーマンスを向上させます。
• driver.Conn インターフェース: データベースドライバが実装する接続のインターフェースです。
• Rows 構造体: クエリ結果の行を反復処理するために使用されます。Next() メソッドで次の行に進み、Scan() メソッドで現在の行のデータをGoの変数に読み込みます。Rows オブジェクトは、すべての行が処理された後、またはエラーが発生した場合に Close() メソッドで明示的にクローズする必要があります。これを怠ると、基盤となるデータベース接続がリークする可能性があります。
• Row 構造体: 単一の行を返すクエリ（例: QueryRow）の結果を表します。Row は Rows のラッパーであり、通常は Scan() メソッドを直接呼び出して結果を取得します。
• driver.ErrBadConn: ドライバがデータベース接続が不良状態（例: ネットワークエラー、サーバー側の接続切断など）であることを示すために返すエラーです。database/sql パッケージは、このエラーを受け取ると、その接続をプールから削除し、再利用しないようにします。
• *RawBytes: database/sql パッケージで提供される特殊な型で、データベースから読み込んだ生のバイトデータを保持するために使用されます。通常、Rows.Scan() や Row.Scan() で使用されますが、そのライフサイクル管理には注意が必要です。特に Row.Scan() の場合は、Rows オブジェクトがすぐにクローズされるため、*RawBytes が指すメモリが解放される可能性があり、不適切な使用は問題を引き起こすことがあります。

ミューテックス (sync.Mutex)

Go言語の sync.Mutex は、共有リソースへのアクセスを同期するための排他ロックメカニズムです。複数のGoroutineが同時に共有データにアクセスする際に、データの破損や競合状態を防ぐために使用されます。Lock() メソッドでロックを取得し、Unlock() メソッドでロックを解放します。defer ステートメントと組み合わせて使用されることが多く、関数の終了時に確実にロックが解放されるようにします。

defer ステートメント

Go言語の defer ステートメントは、そのステートメントを含む関数がリターンする直前に、指定された関数呼び出しを実行することを保証します。これは、リソースの解放（ファイルハンドル、ロック、データベース接続など）やクリーンアップ処理に非常に便利です。

技術的詳細

このコミットは、database/sql パッケージの内部実装における複数の繊細な問題を解決しています。

1. SetMaxIdleConns の修正:

   • 変更前は、SetMaxIdleConns 関数内でアイドル接続をクローズする際に、db.mu ミューテックスを保持したまま go dc.Close() を呼び出していました。dc.Close() は内部で db.mu を再取得しようとする可能性があり、これがデッドロックを引き起こす可能性がありました。
   • 修正後は、クローズすべき接続を一時的なスライス closing に集め、db.mu.Unlock() でミューテックスを解放した後に、別のループでこれらの接続を順次クローズするように変更されました。これにより、dc.Close() が db.mu を取得する必要がある場合でも、デッドロックが発生するリスクがなくなりました。また、go dc.Close() を削除することで、接続のクローズが非同期ではなく同期的に行われるようになり、SetMaxIdleConns が完了した時点でアイドル接続が確実にクローズされている状態になります。

2. putConn における numOpen の二重デクリメント修正:

   • putConn 関数は、使用済みの接続をプールに戻すか、不良接続として破棄する役割を担います。
   • 変更前は、接続が driver.ErrBadConn と判断された場合、putConn 内で db.numOpen-- を実行していました。しかし、その後 dc.Close() が呼び出され、dc.Close() の内部（具体的には finalClose メソッド）でも db.numOpen-- が実行されるため、結果として numOpen が二重にデクリメントされていました。
   • 修正後は、driver.ErrBadConn の場合に putConn 内での db.numOpen-- が削除されました。これにより、numOpen のデクリメントは dc.Close() を通じて一度だけ行われるようになり、正確なオープン接続数が維持されるようになりました。

3. Row.Scan() における Rows リークの修正:

   • Row.Scan() は内部で Rows オブジェクトを使用し、単一の行を読み取ります。この Rows オブジェクトは、データが読み取られた後にクローズされる必要があります。
   • 変更前は、defer r.rows.Close() が *RawBytes の不適切な使用をチェックする if ブロックの後に配置されていました。このため、*RawBytes のチェックでエラーが早期に返された場合、r.rows.Close() が呼び出されず、Rows オブジェクトとそれに紐づくデータベース接続がリークする可能性がありました。
   • 修正後は、defer r.rows.Close() が関数の冒頭に移動されました。これにより、Row.Scan() がどのようなパスで終了しても、r.rows.Close() が確実に呼び出されるようになり、接続リークが防止されます。

4. 接続リーク検出テストの追加:

   • sql_test.go の closeDB ヘルパー関数に、db.numOpen がゼロであることを確認するアサーションが追加されました。これにより、各テストの終了時に開いているデータベース接続が残っていないか（つまり、リークが発生していないか）を自動的にチェックできるようになりました。これは、将来的な接続リークバグの早期発見に非常に有効です。
   • TestRowsColumns テストにも、明示的に rows.Close() を呼び出すコードが追加されました。これは、Row.Scan() のリーク修正と整合性を保ち、テスト自体がリークを引き起こさないようにするためのものです。

これらの変更は、database/sql パッケージの堅牢性と信頼性を大幅に向上させ、Goアプリケーションにおけるデータベース接続管理の安定性を確保します。

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

src/pkg/database/sql/sql.go

--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -504,7 +504,6 @@ func (db *DB) maxIdleConnsLocked() int {
 // If n <= 0, no idle connections are retained.
 func (db *DB) SetMaxIdleConns(n int) {
  	db.mu.Lock()
- 	defer db.mu.Unlock()
  	if n > 0 {
  		db.maxIdle = n
  	} else {
@@ -515,11 +514,16 @@ func (db *DB) SetMaxIdleConns(n int) {
  	if db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen {
  		db.maxIdle = db.maxOpen
  	}\n+\tvar closing []*driverConn
  	for db.freeConn.Len() > db.maxIdleConnsLocked() {
  		dc := db.freeConn.Back().Value.(*driverConn)
  		dc.listElem = nil
  		db.freeConn.Remove(db.freeConn.Back())
-\t\tgo dc.Close()\n+\t\tclosing = append(closing, dc)\n+\t}\n+\tdb.mu.Unlock()\n+\tfor _, c := range closing {\n+\t\tc.Close()\n  	}\n }\n \n@@ -743,8 +747,8 @@ func (db *DB) putConn(dc *driverConn, err error) {
  	if err == driver.ErrBadConn {
  		// Don\'t reuse bad connections.
  		// Since the conn is considered bad and is being discarded, treat it
- 		// as closed. Decrement the open count.\n- 		db.numOpen--\n+\t\t// as closed. Don\'t decrement the open count here, finalClose will\n+\t\t// take care of that.\n  		db.maybeOpenNewConnections()\n  		db.mu.Unlock()\n  		dc.Close()\n@@ -1607,13 +1611,13 @@ func (r *Row) Scan(dest ...interface{}) error {
  	// from Next will not be modified again.\" (for instance, if
  	// they were obtained from the network anyway) But for now we
  	// don\'t care.\n+\tdefer r.rows.Close()\n  	for _, dp := range dest {\n  		if _, ok := dp.(*RawBytes); ok {\n  			return errors.New(\"sql: RawBytes isn\'t allowed on Row.Scan\")
  		}\n  	}\n  \n-\tdefer r.rows.Close()\n  	if !r.rows.Next() {\n  		return ErrNoRows
  	}\n```

### `src/pkg/database/sql/sql_test.go`

```diff
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -94,6 +94,12 @@ func closeDB(t testing.TB, db *DB) {
  	if err != nil {
  		t.Fatalf(\"error closing DB: %v\", err)
  	}\n+\tdb.mu.Lock()\n+\tcount := db.numOpen\n+\tdb.mu.Unlock()\n+\tif count != 0 {\n+\t\tt.Fatalf(\"%d connections still open after closing DB\", db.numOpen)\n+\t}\n }\n \n // numPrepares assumes that db has exactly 1 idle conn and returns\n@@ -246,6 +252,9 @@ func TestRowsColumns(t *testing.T) {
  	if !reflect.DeepEqual(cols, want) {
  		t.Errorf(\"got %#v; want %#v\", cols, want)
  	}\n+\tif err := rows.Close(); err != nil {\n+\t\tt.Errorf(\"error closing rows: %s\", err)\n+\t}\n }\n \n func TestQueryRow(t *testing.T) {

コアとなるコードの解説

src/pkg/database/sql/sql.go の変更点

1. SetMaxIdleConns 関数:

   • defer db.mu.Unlock() が関数の冒頭から削除されました。これは、関数全体でロックを保持するのではなく、アイドル接続をクローズする処理の前にロックを一時的に解放する必要があるためです。
   • var closing []*driverConn が追加され、クローズすべき接続を一時的に保持するためのスライスが宣言されました。
   • アイドル接続をクローズするループ内で、go dc.Close() が closing = append(closing, dc) に変更されました。これにより、接続のクローズが非同期で行われるのではなく、closing スライスに接続が追加されるだけになります。
   • ループの直後に db.mu.Unlock() が追加され、アイドル接続のリスト操作が完了した時点でミューテックスが解放されます。
   • for _, c := range closing { c.Close() } という新しいループが追加され、ミューテックスが解放された後に、収集されたすべての接続が同期的にクローズされます。この変更により、dc.Close() が db.mu を取得しようとしてもデッドロックが発生しなくなります。

2. putConn 関数:

   • if err == driver.ErrBadConn ブロック内で、db.numOpen-- の行が削除されました。
   • 関連するコメントも「finalClose がそれを処理する」という内容に更新されました。
   • この変更により、不良接続が破棄される際に numOpen が二重にデクリメントされる問題が解決されます。dc.Close() が呼び出されると、その内部で finalClose が実行され、そこで一度だけ numOpen がデクリメントされるため、正確なカウントが維持されます。

3. Row.Scan 関数:

   • defer r.rows.Close() の位置が、for _, dp := range dest ループの直前から関数の冒頭に移動されました。
   • この変更により、Row.Scan() 関数がどのような理由（例: *RawBytes の不適切な使用によるエラー）で早期にリターンしても、r.rows.Close() が確実に呼び出されるようになり、Rows 構造体とそれに紐づくデータベース接続のリークが防止されます。

src/pkg/database/sql/sql_test.go の変更点

1. closeDB 関数:

   • db.numOpen の値をロックして取得し、それがゼロであることをアサートするコードブロックが追加されました。
   • if count != 0 { t.Fatalf("%d connections still open after closing DB", db.numOpen) } という行が、テスト終了時に開いている接続が残っている場合にテストを失敗させるようにします。
   • この変更により、テストスイート全体でデータベース接続のリークを自動的に検出できるようになります。

2. TestRowsColumns 関数:

   • if err := rows.Close(); err != nil { t.Errorf("error closing rows: %s", err) } という行が追加されました。
   • この変更は、TestRowsColumns テスト内で明示的に rows.Close() を呼び出すことで、テスト自体が接続リークを引き起こさないようにし、Row.Scan() のリーク修正と整合性を保つためのものです。

これらのコード変更は、database/sql パッケージの内部的な堅牢性を高め、接続管理の正確性を保証し、潜在的なリソースリークを防ぐ上で非常に重要です。

関連リンク

• Go Code Review: https://golang.org/cl/14642044
• Go Issue: #6593 (このコミットが解決した問題のトラッキング)

参考にした情報源リンク

• Go database/sql package documentation: https://pkg.go.dev/database/sql
• Go sync package documentation: https://pkg.go.dev/sync
• Go defer statement: https://go.dev/blog/defer-panic-recover
• Understanding database/sql connection pool: https://www.alexedwards.net/blog/configuring-go-sql-connections (一般的な情報源として)
• Go database/sql and RawBytes: https://pkg.go.dev/database/sql#RawBytes (一般的な情報源として)