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

Fix startAutoCloseIdleConnections cause goroutine leak #1011

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

YenchangChan
Copy link
Contributor

Summary

In the submission of #999, if a connection idle time exceeds ConnMaxLifetime, the connection will be closed, thus introducing the following code:

func (ch *clickhouse) startAutoCloseIdleConnections() {
	ticker := time.NewTicker(ch.opt.ConnMaxLifetime)
	defer ticker.Stop()

        for {
		select {
		case <-ticker.C:
			ch.closeIdleExpired()
		}
	}
}

But this code is seriously bugged because defer ticker.Stop() will NEVER be executed, and the for loop will NEVER exit! Therefore, every time a new connection is created using the Open function, a new goroutine is created and can never be destroyed.
My test code is as follows:

package main

import (
        "context"
        "fmt"
        "net"
        "runtime"
        "time"

        "github.com/ClickHouse/clickhouse-go/v2"
        "github.com/ClickHouse/clickhouse-go/v2/lib/driver"
)

func connect() (driver.Conn, error) {
        var (
                ctx       = context.Background()
                conn, err = clickhouse.Open(&clickhouse.Options{
                        Addr: []string{net.JoinHostPort("localhost", "9000")},
                        Auth: clickhouse.Auth{
                                Database: "default",
                                Username: "default",
                                Password: "123456",
                        },
                })
        )

        if err != nil {
                return nil, err
        }

        if err := conn.Ping(ctx); err != nil {
                conn.Close()
                return nil, err
        }
        return conn, nil
}

func main() {
        baseGoroutine := runtime.NumGoroutine()
        conns := make([]driver.Conn, 100)
        for i := 0; i < 100; i++ {
                conn, err := connect()
                if err == nil {
                        conns[i] = conn
                }
        }

        fmt.Printf("Running goroutine: %d\n", runtime.NumGoroutine() - baseGoroutine)
        for _, conn := range conns {
                if conn != nil {
                        conn.Close()
                } else {
                        fmt.Println("conn is null")
                }
        }
        time.Sleep(100 * time.Millisecond)
        fmt.Printf("final goroutine: %d\n", runtime.NumGoroutine() - baseGoroutine)
}

I create 100 connections and close them without doing anything, but there are still 100 goroutines left that cannot be destroyed.

The code execution result is as follows:

# go run main.go 
Running goroutine: 100
final goroutine: 100

If connections can be created successfully, more professional developers will maintain a pool of connections, which usually does not result in unlimited growth of connections, and goroutine leakage is manageable. But if the Open call succeeds and the Ping fails, then the goroutine is out of control forever.

For example, to connect to clickhouse in a scheduled task, and then process some business logic, but the connection fails, there will be an infinite number of coroutines, which cannot be destroyed unless you restart your program.
My PR will fix #999 goroutines leak problem, when the Close function is invoked, to signal startAutoCloseIdleConnections function exit.

Checklist

  • Unit and integration tests covering the common scenarios were added

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2023

CLA assistant check
All committers have signed the CLA.

@jkaflik jkaflik changed the title fix: startAutoCloseIdleConnections cause goroutine leak Fix startAutoCloseIdleConnections cause goroutine leak Jul 5, 2023
@jkaflik jkaflik merged commit f476287 into ClickHouse:main Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants