ClickHouse / ClickHouse/clickhouse-go

connPool.Close() can deadlock with runDrainPool ticker tick (mutex held across finished-wait)

Open
#1,990 2 comments 2 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
3.3k
Forks
680
Avg merge
2d 3h
Merged PRs (30d)
14

Description

## Summary

`connPool.Close()` can **deadlock permanently** against the background `runDrainPool` goroutine: `Close` holds the pool's write mutex while blocking on `<-i.finished`, while the drain goroutine — if it has just woken on its ticker and is blocked acquiring that same mutex — can only observe the close signal *after* it finishes the ticker branch, which requires the mutex `Close` is holding and will not release until `<-i.finished` returns. Circular wait; neither side makes progress.

## Location

- File: [`conn_pool.go`](https://github.com/ClickHouse/clickhouse-go/blob/0c09672ddf7252afcdc32db324a2b53a78113377/conn_pool.go)
- `Close()` (lines 117–133) and `runDrainPool()` (lines 144–160)

```go
func (i *connPool) Close() error {
i.mu.Lock() // (1) writer lock held for the whole function
defer i.mu.Unlock()
if i.closed() { return nil }
close(i.finish)
<-i.finished // (2) waits for the drain goroutine to exit
i.drainPool()
return nil
}

func (i *connPool) runDrainPool() {
defer func() { i.ticker.Stop(); close(i.finished) }()
for {
select {
case <-i.ticker.C:
i.mu.Lock() // (3) can block behind (1)
i.drainPool()
i.mu.Unlock()
case <-i.finish:
return
}
}
}
```

## Problem

Deadlock sequence:

1. The drain goroutine wakes on `ticker.C` and calls `i.mu.Lock()` at line 153.
2. Before it acquires the mutex, `Close()` barges in and takes it (`sync.Mutex` in normal mode permits barging ahead of queued waiters).
3. `Close()` sets `closed` state via `close(i.finish)` and then blocks on `<-i.finished` (line 127) **while still holding `mu`**.
4. The drain goroutine, parked at line 153, cannot acquire the mutex because `Close()` holds it; it therefore never reaches the `case <-i.finish:` branch, so its deferred `close(i.finished)` never runs.
5. `Close()` never returns; every subsequent `Get`/`Put` also blocks on the held mutex.

The window is between the goroutine waking on the ticker and acquiring the mutex. It is narrow per event, but the ticker fires once per connection lifetime for the entire process lifetime, and Go's non-FIFO mutex behavior makes the "Close jumps the queue" interleaving realistic rather than theoretical.

## Trigger / Reproduction

Static analysis finding — not confirmed by execution; derived from the control flow above at `main` (`0c09672d`). Conceptual repro:

- Create a client with a short `ConnMaxLifetime` (to make the ticker fire frequently).
- Concurrently call `clickhouse.Close()` (or let finalizers/HTTP-server shutdown do it) in a loop.
- Eventually `Close()` hangs forever with all pool operations stuck.

A deterministic unit test can force the interleaving by injecting a hook/pause between the select wake-up and `mu.Lock()`, or by replacing the ticker channel with a controllable channel and gating `Close()` until the drain goroutine is provably blocked on `Lock` (observable via runtime stack inspection).

## Expected Behavior

Closing the pool should always terminate. The close signal must be observable regardless of where the drain goroutine currently sits, e.g.:

- take the ticker work outside the mutex conflict path: in the ticker branch, use a non-blocking try-lock or re-check `i.closed()` after acquiring the lock and bail out;
- or have `Close()` release the mutex before waiting: set closed flag under lock, unlock, `close(finish)`, wait for `finished`, then drain under lock.

## Actual Behavior

Permanent hang of `Close()` and of all subsequent pool operations when the unlucky interleaving occurs.

## Impact

Any code path shutting down a `clickhouse.OpenDB` client (server shutdown, tests, CLI tools) can freeze indefinitely with all connections leaked-busy; goroutine dumps show `Close` parked on `.finished` and `runDrainPool` parked on `Lock`.

## Evidence

- Mutex hold spans the `<-i.finished` wait: lines 118–127.
- Drain goroutine's only exit check happens in `select`; the ticker branch unconditionally locks the same mutex first: lines 150–158.
- `closed()` is checked nowhere inside the ticker branch before locking.

Contributor guide

Open the contributing guide

Research direction

Start in conn_pool.go by tracing Close() and runDrainPool() around the mutex, finish channel, and ticker branches. Build a deterministic regression test for the reported interleaving, then verify that pool shutdown returns and subsequent pool operations are not left blocked.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.