Possible goroutine leak in metricbeat sql module when database is not available

[simioa]

• Version: metricbeat 7.12.1
• Operating System: Fedora 34
• Discuss Forum URL: https://discuss.elastic.co/t/possible-goroutine-leak-in-sql-module-when-database-is-not-available/273850

Hello,

we're using the metricbeat sql module to gather some metrics. During Downtime of our Database, I observed slowly increasing memory consumption from metricbeat.
I tried to reproduce this with the following metricbeat config with a non-existent Database:

• Steps to Reproduce:

1. Create SQL Config pointing to a non-existent Database
   Using example config from the metricbeat reference:

metricbeat.modules:
- module: sql
  metricsets:
    - query
  period: 10s
  hosts: ["root:root@tcp(localhost:3306)/ps"]

  driver: "mysql"
  sql_query: "SHOW GLOBAL STATUS LIKE 'Innodb_system%'"
  sql_response_format: variables

2. Let it run for a few minutes/hours - While metricbeat was running, I could see a steady increase in memory consumption from metricbeat.

I restarted metricbeat with the httpprof parameter and could observe that many database/sql.(*DB).connectionOpener goroutines are being created.

goroutine profile: total 10395
10305 @ 0x1f2cd25 0x1f3ce8f 0x39018d5 0x1f63e41
#	0x39018d4	database/sql.(*DB).connectionOpener+0xf4	/usr/local/go/src/database/sql/sql.go:1126 

With debug=2

goroutine 2004 [select, 14 minutes]:
database/sql.(*DB).connectionOpener(0xc000df6dd0, 0x5fa9520, 0xc00096b940)
	/usr/local/go/src/database/sql/sql.go:1126 +0xf5
created by database/sql.OpenDB
	/usr/local/go/src/database/sql/sql.go:740 +0x12a

After some debugging I think I figured out what happens.

In

beats/metricbeat/helper/sql/sql.go

Lines 53 to 56 in 651a2ad

	err = dbx.Ping()
	if err != nil {
	return nil, errors.Wrap(err, "testing connection")
	}

there is a check to test if the connection to the database works and if not, returns an error - but in case there is an error, the Database Object is never closed and leaking the goroutine.

To test my assumption, I replaced the code

if err != nil {
    return nil, errors.Wrap(err, "testing connection")
}

with

if err != nil {
	if dbx != nil {
		dbx.Close()
	}
	return nil, errors.Wrap(err, "testing connection")
}

After replacing the code, I could not observe further memory growth or goroutine creations.

Can you please check if my assumption is correct or if this is even a bug?

[elasticmachine]

Pinging @elastic/integrations (Team:Integrations)

[jsoriano]

@simioa thanks for reporting this! I think you are right, NewDBClient should close the connection if it fails to ping. Would you like to open a PR with your fix?