Skip to content

Commit a1746d7

Browse files
wudidapaopaoclaude
andcommitted
fix: address review findings on concurrent-connections
Resolves issues raised in the PR #38 review: - session: key sessions by resolved physical path (strip "file:" scheme, drop "?params", make absolute), matching what the native layer opens, so the same data path written different ways ("db" / "file:db" / "file:db?p=v" / ":memory:" / "file::memory:") is recognized as the same path instead of raising a spurious "conflicting path" error. Resolution is OS-aware (Windows drive-letter paths). Path()/Cleanup now use the resolved directory, fixing a wrong-dir RemoveAll that left the real data directory on disk for file:/param DSNs. - session: gate Cleanup()'s RemoveAll on the refcount reaching zero so it no longer deletes a shared data directory while sibling sessions on the same path are still live. - session: guard the native connection with a per-Session RWMutex. Query/ QueryStream take a read lock and return an error on a closed session; Close/Cleanup take the write lock and wait for in-flight queries before freeing the connection. Fixes a use-after-free / crash when one session is queried and closed concurrently. purego connection.Close now nils its handle (idempotent; no double-free). - driver: Driver.Open ties the connector's keeper session to the returned conn so closing the conn also releases the keeper; previously the keeper (a native connection + registry refcount + temp dir) leaked on every direct Driver.Open call. - wrapper: ephemeralSession only retries with NewSession() on a genuine one-data-path-per-process conflict, surfacing unrelated errors instead of masking them. - docs: chdb.md Query/QueryStream now describe attach-to-active-path semantics. Tests: add before/after coverage for each fix (path spellings, shared-dir cleanup, refcounted temp lifecycle, conflict-then-recover, Driver.Open keeper balance, query-after-close, concurrent query/close under -race); add defer db.Close() to driver tests that previously leaked *sql.DB; add ActiveSessionRefs() for refcount assertions. go test ./... -race passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent da57b21 commit a1746d7

8 files changed

Lines changed: 506 additions & 66 deletions

File tree

chdb-purego/chdb.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,14 @@ func newChdbConn(conn *chdb_connection) ChdbConn {
112112
}
113113

114114
// Close implements ChdbConn.
115+
//
116+
// Close is idempotent: it nils the underlying handle after freeing it so a
117+
// second Close (or a Close racing a Query that checks for a nil handle) does
118+
// not double-free the native connection.
115119
func (c *connection) Close() {
116120
if c.conn != nil {
117121
chdbCloseConn(c.conn)
122+
c.conn = nil
118123
}
119124
}
120125

chdb.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import "github.com/chdb-io/chdb-go/chdb"
2828
func Query(queryStr string, outputFormats ...string) (result chdbpurego.ChdbResult, err error)
2929
```
3030

31-
Query calls query\_conn with a default in\-memory session and default output format of "CSV" if not provided.
31+
Query runs a one\-shot query and returns the materialized result \(default output format "CSV"\). chDB allows only one data path per process, so if a session is already open this helper attaches to that session's data path; otherwise it uses an in\-memory database.
3232

3333
<a name="QueryStream"></a>
3434
## func [QueryStream](<https://github.com/s0und0fs1lence/chdb-go/blob/main/chdb/wrapper.go#L23>)
@@ -37,7 +37,7 @@ Query calls query\_conn with a default in\-memory session and default output for
3737
func QueryStream(queryStr string, outputFormats ...string) (result chdbpurego.ChdbStreamResult, err error)
3838
```
3939

40-
Query calls query\_conn with a default in\-memory session and default output format of "CSV" if not provided.
40+
QueryStream is like Query but returns a streaming result that can be read in chunks, for large datasets that should not be fully materialized in memory. Like Query, it attaches to an already\-open session's data path, or uses an in\-memory database when none is open.
4141

4242
<a name="Session"></a>
4343
## type [Session](<https://github.com/s0und0fs1lence/chdb-go/blob/main/chdb/session.go#L14-L19>)

chdb/driver/driver.go

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,25 @@ func (d Driver) Open(name string) (driver.Conn, error) {
293293
if err != nil {
294294
return nil, err
295295
}
296-
return cc.Connect(context.Background())
296+
c, err := cc.Connect(context.Background())
297+
if err != nil {
298+
// Connect failed; release the keeper session NewConnect just opened so
299+
// it does not leak (database/sql would normally own and close cc).
300+
if closer, ok := cc.(*connector); ok {
301+
_ = closer.Close()
302+
}
303+
return nil, err
304+
}
305+
// On the sql.Open path, database/sql keeps the connector and calls
306+
// connector.Close() on db.Close(). A direct Driver.Open caller discards the
307+
// connector, so tie the keeper session's lifetime to the returned
308+
// connection: closing the conn also releases the keeper.
309+
if cn, ok := c.(*conn); ok {
310+
if cnr, ok := cc.(*connector); ok {
311+
cn.connector = cnr
312+
}
313+
}
314+
return c, nil
297315
}
298316

299317
// OpenConnector expects the same format as driver.Open
@@ -312,6 +330,10 @@ type conn struct {
312330
useUnsafe bool
313331
isStreaming bool
314332
session *chdb.Session
333+
// connector is set only on the legacy Driver.Open path (not the sql.Open
334+
// path, where database/sql owns and closes the connector). When set, Close
335+
// also releases the connector's keeper session.
336+
connector *connector
315337

316338
QueryFun queryHandle
317339
streamFun queryStream
@@ -334,6 +356,12 @@ func (c *conn) Close() error {
334356
c.session.Close()
335357
c.session = nil
336358
}
359+
// Only set on the Driver.Open path; releases the keeper that pins the data
360+
// path/temp dir for the connector.
361+
if c.connector != nil {
362+
_ = c.connector.Close()
363+
c.connector = nil
364+
}
337365
return nil
338366
}
339367

chdb/driver/driver_open_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package chdbdriver
2+
3+
import (
4+
"database/sql"
5+
"testing"
6+
7+
"github.com/chdb-io/chdb-go/chdb"
8+
)
9+
10+
// TestDriverOpenNoKeeperLeak verifies that the legacy Driver.Open path does not
11+
// leak the connector's keeper session. database/sql owns the connector on the
12+
// sql.Open path and calls connector.Close(), but a direct Driver.Open(name)
13+
// discards the connector, so closing the returned conn must release the keeper
14+
// too. Pre-fix the keeper session (a native connection + a registry refcount)
15+
// leaks on every Open call.
16+
func TestDriverOpenNoKeeperLeak(t *testing.T) {
17+
baseline := chdb.ActiveSessionRefs()
18+
19+
c, err := Driver{}.Open("session=" + session.ConnStr())
20+
if err != nil {
21+
t.Fatalf("Driver.Open failed: %s", err)
22+
}
23+
if err := c.Close(); err != nil {
24+
t.Fatalf("conn.Close failed: %s", err)
25+
}
26+
27+
if got := chdb.ActiveSessionRefs(); got != baseline {
28+
t.Fatalf("Driver.Open leaked sessions: refs=%d, baseline=%d (keeper not released on conn.Close)", got, baseline)
29+
}
30+
}
31+
32+
// TestDbCloseReleasesRefs verifies the database/sql path balances refcounts:
33+
// opening a *sql.DB, running queries across multiple pooled connections, and
34+
// closing it returns the registry to its baseline (keeper + per-conn sessions
35+
// all released).
36+
func TestDbCloseReleasesRefs(t *testing.T) {
37+
baseline := chdb.ActiveSessionRefs()
38+
39+
db, err := sql.Open("chdb", "session="+session.ConnStr())
40+
if err != nil {
41+
t.Fatalf("open db failed: %s", err)
42+
}
43+
db.SetMaxOpenConns(4)
44+
45+
for i := 0; i < 8; i++ {
46+
var n int
47+
if err := db.QueryRow("SELECT count() FROM numbers(10)").Scan(&n); err != nil {
48+
t.Fatalf("query failed: %s", err)
49+
}
50+
if n != 10 {
51+
t.Fatalf("got %d want 10", n)
52+
}
53+
}
54+
55+
if err := db.Close(); err != nil {
56+
t.Fatalf("db.Close failed: %s", err)
57+
}
58+
if got := chdb.ActiveSessionRefs(); got != baseline {
59+
t.Fatalf("db.Close left refs leaked: refs=%d, baseline=%d", got, baseline)
60+
}
61+
}

chdb/driver/driver_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ func TestDb(t *testing.T) {
5050
if err != nil {
5151
t.Fatalf("open db fail, err:%s", err)
5252
}
53+
defer db.Close()
5354
if db.Ping() != nil {
5455
t.Fatalf("ping db fail")
5556
}
@@ -88,6 +89,7 @@ func TestDbWithCompiledArgs(t *testing.T) {
8889
if err != nil {
8990
t.Errorf("open db fail, err:%s", err)
9091
}
92+
defer db.Close()
9193
if db.Ping() != nil {
9294
t.Errorf("ping db fail")
9395
}
@@ -170,6 +172,7 @@ func TestDbWithSession(t *testing.T) {
170172
if err != nil {
171173
t.Fatalf("open db fail, err: %s", err)
172174
}
175+
defer db.Close()
173176
if db.Ping() != nil {
174177
t.Fatalf("ping db fail, err: %s", err)
175178
}
@@ -217,6 +220,7 @@ func TestDbWithConnection(t *testing.T) {
217220
if err != nil {
218221
t.Fatalf("open db fail, err: %s", err)
219222
}
223+
defer db.Close()
220224
if db.Ping() != nil {
221225
t.Fatalf("ping db fail, err: %s", err)
222226
}
@@ -251,6 +255,7 @@ func TestDbWithConnectionSqlDriverOnly(t *testing.T) {
251255
if err != nil {
252256
t.Fatalf("open db fail, err: %s", err)
253257
}
258+
defer db.Close()
254259
if db.Ping() != nil {
255260
t.Fatalf("ping db fail, err: %s", err)
256261
}
@@ -309,6 +314,7 @@ func TestQueryRow(t *testing.T) {
309314
if err != nil {
310315
t.Fatalf("open db fail, err: %s", err)
311316
}
317+
defer db.Close()
312318
if db.Ping() != nil {
313319
t.Fatalf("ping db fail, err: %s", err)
314320
}
@@ -339,6 +345,7 @@ func TestExec(t *testing.T) {
339345
if err != nil {
340346
t.Fatalf("open db fail, err: %s", err)
341347
}
348+
defer db.Close()
342349
if db.Ping() != nil {
343350
t.Fatalf("ping db fail, err: %s", err)
344351
}

0 commit comments

Comments
 (0)