Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broken connection is returned to pool after context has been canceled #519

Closed
JakobJoonas opened this issue Jun 17, 2022 · 2 comments
Closed

Comments

@JakobJoonas
Copy link

JakobJoonas commented Jun 17, 2022

Description

When context is canceled during the query, then it's not cleaned properly as its using go-sql-driver v1.5.0 instead of v1.6.0 (v1.6.0 supports IsValid(), which validates the connection before returning it to pool) go-sql-driver PR

Currently this step is skipped and pool gets canceled connections.

Steps to Reproduce

		dsn := fmt.Sprintf("root:root@tcp(%s:%d)/", address, port)

		// open connection with new relic wrapper
		db, err := sql.Open("nrmysql", dsn)
		assert.NoError(t, err)

		// Setup timeout of a query
		ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*50)
		defer cancel()

		// Get the connection
		conn, err := db.Conn(ctx)
		assert.NoError(t, err)

		// Execute the request, context will be canceled before this query manages to execute
		_, err = conn.ExecContext(ctx, "SELECT SLEEP(1)")
		assert.Error(t, err)

		// Close the connection
		// This only releases the connection, however does not validate if the connection is valid
		err = conn.Close()
		assert.NoError(t, err)

		// Pick a new connection from the pool (as connections are reused, then the previous one is picked)
		conn, err = db.Conn(context.Background())
		assert.NoError(t, err)

		// As we are reusing the broken connection, then this ping will fail.
		err = conn.PingContext(context.Background())
		assert.NoError(t, err)

		// Now change the driver from `nrmysql` to `mysql`
		// Make sure that go-sql-driver version is v1.6.0 and rerun the test, it should pass.

Expected Behavior

Before connection is passed back to the pool, it should be validated if it has been canceled

Your Environment

Go 1.17
github.com/go-sql-driver/mysql v1.6.0
github.com/newrelic/go-agent/v3/integrations/nrmysql v1.2.1

@iamemilio
Copy link
Contributor

Thanks for filing this bug @JakobJoonas. It's always super helpful when there is a good reproducer attached. We will take a look at this and patch it up as soon as we can.

@iamemilio iamemilio added this to Awaiting User Input in Go Engineering Board via automation Jul 12, 2022
@iamemilio
Copy link
Contributor

@JakobJoonas we totally lost sight of this. If I understand correctly, upgrading to github.com/go-sql-driver/mysql v1.6.0 should resolve this problem, right?

iamemilio added a commit that referenced this issue Sep 20, 2022
Fix #519: bump sql driver to v1.6.0
Go Engineering Board automation moved this from Awaiting User Input to Done Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants