ClickHouse / ClickHouse/clickhouse-go
connPool.Close() can deadlock with runDrainPool ticker tick (mutex held across finished-wait)
- 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
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