Nil panic when using transactions on single connection

[funvit]

Observed

1. One connection to clickhouse
2. Two transactions, one by one
3. First transaction have an intentional mistake
4. Second transaction will fail with panic on tx.ExecContext (or tx.Exec)

Expected behaviour

Second transaction must not panics.

Code example

package code

// your code snippet here
import (
	"context"
	"database/sql"
	"fmt"
	"net/http"
	"testing"
	"time"

	_ "github.com/ClickHouse/clickhouse-go/v2"
	"github.com/pkg/errors"
	"github.com/stretchr/testify/require"
	"github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
)

func Test_TwoTxsOnSingleConn(t *testing.T) {
	ctx := context.Background()

	//
	// Prepare environment
	//

	// Create a ClickHouse container
	req := testcontainers.ContainerRequest{
		Image:        "clickhouse/clickhouse-server:22.3.2.2",
		ExposedPorts: []string{"8123/tcp", "9000/tcp"},
		WaitingFor: wait.ForAll(
			wait.ForHTTP("/ping").WithPort("8123/tcp").WithStatusCodeMatcher(func(status int) bool {
				return status == http.StatusOK
			}).WithStartupTimeout(30 * time.Second),
		),
	}

	// Start the container
	clickhouseContainer, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
		ContainerRequest: req,
		Started:          true,
	})
	if err != nil {
		t.Fatalf("Failed to start ClickHouse container: %v", err)
	}
	defer clickhouseContainer.Terminate(ctx) // Ensure the container is terminated after the test

	// Get the container's host and port
	host, err := clickhouseContainer.Host(ctx)
	if err != nil {
		t.Fatalf("Failed to get container host: %v", err)
	}
	port, err := clickhouseContainer.MappedPort(ctx, "9000/tcp")
	if err != nil {
		t.Fatalf("Failed to get mapped port: %v", err)
	}

	// Create a DSN for ClickHouse
	dsn := fmt.Sprintf("clickhouse://%s:%s/default?username=default&password=", host, port.Port())

	// Open a connection to ClickHouse
	db, err := sql.Open("clickhouse", dsn)
	if err != nil {
		t.Fatalf("Failed to connect to ClickHouse: %v", err)
	}
	defer db.Close() // Ensure the connection is closed when done

	singleConn, err := db.Conn(ctx)
	if err != nil {
		t.Fatalf("Get single conn from pool: %v", err)
	}

	//
	// Begin testing
	//
	tx1 := func(c *sql.Conn) error {
		tx, err := c.BeginTx(ctx, nil)
		if err != nil {
			return errors.Wrap(err, "begin tx")
		}
		defer tx.Rollback()

		_, err = tx.ExecContext(ctx, `
CREATE TABLE IF NOT EXISTS test_table
ON CLUSTER my
(id UInt32, name String)
ENGINE = MergeTree()
ORDER BY id`)
		if err != nil {
			return errors.Wrap(err, "create table")
		}

		err = tx.Commit()
		if err != nil {
			return errors.Wrap(err, "commit tx")
		}

		return nil
	}
	require.Error(t, tx1(singleConn), "expected error due to cluster is not configured")

	tx2 := func(c *sql.Conn) error {
		tx, err := c.BeginTx(ctx, nil)
		if err != nil {
			return errors.Wrap(err, "begin tx")
		}
		defer tx.Rollback()

		// NOTE: this will panics for now...
		_, err = tx.ExecContext(ctx, "INSERT INTO test_table (id, name) VALUES (?, ?)", 1, "test_name")
		if err != nil {
			return errors.Wrap(err, "failed to insert record")
		}
		err = tx.Commit()
		if err != nil {
			return errors.Wrap(err, "commit tx")
		}

		return nil
	}
	require.NotPanics(
		t,
		func() {
			require.Error(t, tx2(singleConn), "expected error due to database is not exists")
		},
		"must not panics",
	)
}

Error log

Panic value:	runtime error: invalid memory address or nil pointer dereference
Panic stack:	goroutine 39 [running]:
runtime/debug.Stack()
    /opt/homebrew/opt/[email protected]/libexec/src/runtime/debug/stack.go:24 +0x64
github.com/stretchr/testify/assert.didPanic.func1()
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/stretchr/testify/assert/assertions.go:1196 +0x80
panic({0x1057bb9c0, 0x105deda40})
    /opt/homebrew/opt/[email protected]/libexec/src/runtime/panic.go:884 +0x204
github.com/ClickHouse/ch-go/proto.(*Buffer).PutUInt8(...)
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/ClickHouse/ch-go/proto/buffer.go:97
github.com/ClickHouse/ch-go/proto.(*Buffer).PutByte(...)
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/ClickHouse/ch-go/proto/buffer.go:82
github.com/ClickHouse/clickhouse-go/v2.(*connect).sendQuery(0x140001da1c0, {0x140000e7880, 0x39}, 0x14000396780)
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/ClickHouse/clickhouse-go/v2/conn_send_query.go:28 +0xdc
github.com/ClickHouse/clickhouse-go/v2.(*connect).exec(0x140001da1c0, {0x1058a99e0, 0x14000120018}, {0x1055b6d59, 0x2f}, {0x14000222dc0, 0x2, 0x2})
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/ClickHouse/clickhouse-go/v2/conn_exec.go:43 +0x228
github.com/ClickHouse/clickhouse-go/v2.(*stdDriver).ExecContext(0x14000222d20, {0x1058a99e0, 0x14000120018}, {0x1055b6d59, 0x2f}, {0x14000033bd0, 0x2, 0x14000222d20?})
    /Users/vvfuntikov/Workspace/myproj1/vendor/github.com/ClickHouse/clickhouse-go/v2/clickhouse_std.go:241 +0x2ac
database/sql.ctxDriverExec({0x1058a99e0?, 0x14000120018?}, {0x12d765278?, 0x14000222d20?}, {0x0?, 0x0?}, {0x1055b6d59?, 0x14000222d20?}, {0x14000033bd0?, 0x0?, ...})
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/ctxutil.go:31 +0xac
database/sql.(*DB).execDC.func2()
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/sql.go:1675 +0x12c
database/sql.withLock({0x1058a6be0, 0x1400034d680}, 0x1400043ee08)
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/sql.go:3405 +0x7c
database/sql.(*DB).execDC(0x140003e1d00?, {0x1058a99e0, 0x14000120018}, 0x1400034d680, 0x14000033b80?, {0x1055b6d59, 0x2f}, {0x1400043ef80, 0x2, 0x2})
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/sql.go:1670 +0x19c
database/sql.(*Tx).ExecContext(0x140003e1d00, {0x1058a99e0, 0x14000120018}, {0x1055b6d59, 0x2f}, {0x1400043ef80, 0x2, 0x2})
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/sql.go:2471 +0x88
database/sql.(*Tx).Exec(...)
    /opt/homebrew/opt/[email protected]/libexec/src/database/sql/sql.go:2480

Details

Environment

• clickhouse-go version: v2.17.1
• Interface: database/sql compatible driver
• Go version: v1.20
• Operating system: MacOS Sonoma 14.5
• ClickHouse version: 22.3.2.2
• Is it a ClickHouse Cloud? NO
• ClickHouse Server non-default settings, if any: NO
• CREATE TABLE statements for tables involved: see in test
• Sample data for all these tables, use clickhouse-obfuscator if necessary: NOT USED

Possible fix

In file conn.go, replace c.buffer = nil to c.buffer.Reset():

func (c *connect) close() error {
	if c.closed {
		return nil
	}
	c.closed = true
	c.buffer.Reset()  // this fixes panic !!!
	c.reader = nil
	if err := c.conn.Close(); err != nil {
		return err
	}
	return nil
}

[funvit]

Currently this affects goose-migrator...

[jkaflik]

Hi @funvit
First of all, thank you for the excellent reproducible example. This helps a lot.

clickhouse-go behavior is to handle ClickHouse errors and mark the connection as degraded.
Your solution indeed makes code not panic; however, in my opinion, it shouldn't get to query execution.
Please see my PR #1396 that adds a necessary checks before any connection action is performed in stdlib's driver.