Fix memory leak in SQL helper when database is not available (#26607)

jsoriano · web-flow · commit e4d4f76706d7 · 2021-07-01T13:06:32.000+02:00
Explicitly close SQL connections when connectivity checks fail during their
creation, to avoid leaking resources when a connection is created but it
is not going to be used.
diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
@@ -502,6 +502,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Fix metric grouping for windows/perfmon module {issue}23489[23489] {pull}23505[23505]
 - Major refactor of system/cpu and system/core metrics. {pull}25771[25771]
 - Fix GCP Project ID being ingested as `cloud.account.id` in `gcp.billing` module {issue}26357[26357] {pull}26412[26412]
+- Fix memory leak in SQL module when database is not available. {issue}25840[25840] {pull}26607[26607]
 - Fix aws metric tags with resourcegroupstaggingapi paginator.  {issue}26385[26385] {pull}26443[26443]
 
 *Packetbeat*
diff --git a/metricbeat/helper/sql/sql.go b/metricbeat/helper/sql/sql.go
@@ -52,6 +52,9 @@ func NewDBClient(driver, uri string, l *logp.Logger) (*DbClient, error) {
 	}
 	err = dbx.Ping()
 	if err != nil {
+		if closeErr := dbx.Close(); closeErr != nil {
+			return nil, errors.Wrapf(err, "failed to close with %s, after connection test failed", closeErr)
+		}
 		return nil, errors.Wrap(err, "testing connection")
 	}
 
diff --git a/metricbeat/helper/sql/sql_test.go b/metricbeat/helper/sql/sql_test.go
@@ -18,11 +18,18 @@
 package sql
 
 import (
+	"context"
+	"database/sql"
+	"database/sql/driver"
+	"fmt"
 	"math"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/elastic/beats/v7/libbeat/common"
+	"github.com/elastic/beats/v7/libbeat/tests/resources"
 )
 
 type kv struct {
@@ -186,3 +193,55 @@ func TestToDotKeys(t *testing.T) {
 		t.Fail()
 	}
 }
+
+func TestNewDBClient(t *testing.T) {
+	t.Run("create and close", func(t *testing.T) {
+		goroutines := resources.NewGoroutinesChecker()
+		defer goroutines.Check(t)
+
+		client, err := NewDBClient("dummy", "localhost", nil)
+		require.NoError(t, err)
+
+		err = client.Close()
+		require.NoError(t, err)
+	})
+
+	t.Run("unavailable", func(t *testing.T) {
+		goroutines := resources.NewGoroutinesChecker()
+		defer goroutines.Check(t)
+
+		_, err := NewDBClient("dummy", "unavailable", nil)
+		require.Error(t, err)
+	})
+}
+
+func init() {
+	sql.Register("dummy", dummyDriver{})
+}
+
+type dummyDriver struct{}
+
+func (dummyDriver) Open(name string) (driver.Conn, error) {
+	if name == "error" {
+		return nil, fmt.Errorf("error")
+	}
+
+	return &dummyConnection{name: name}, nil
+}
+
+type dummyConnection struct {
+	name string
+}
+
+// Ensure that this dummy connection implements the pinger interface, used by the helper.
+var _ driver.Pinger = &dummyConnection{}
+
+func (*dummyConnection) Prepare(query string) (driver.Stmt, error) { return nil, nil }
+func (*dummyConnection) Close() error                              { return nil }
+func (*dummyConnection) Begin() (driver.Tx, error)                 { return nil, nil }
+func (c *dummyConnection) Ping(context.Context) error {
+	if c.name == "unavailable" {
+		return fmt.Errorf("database unavailable")
+	}
+	return nil
+}

Original file line number	Diff line number	Diff line change
`@@ -52,6 +52,9 @@ func NewDBClient(driver, uri string, l logp.Logger) (DbClient, error) {`
`52`	`52`	`}`
`53`	`53`	`err = dbx.Ping()`
`54`	`54`	`if err != nil {`
	`55`	`+ if closeErr := dbx.Close(); closeErr != nil {`
	`56`	`+ return nil, errors.Wrapf(err, "failed to close with %s, after connection test failed", closeErr)`
	`57`	`+ }`
`55`	`58`	`return nil, errors.Wrap(err, "testing connection")`
`56`	`59`	`}`
`57`	`60`