Skip to content

Commit 1b2f6ad

Browse files
author
xianml
committed
review: gracefully degrade on libchdb builds without the signal handler API
Per review feedback: pre-check chdb_set_signal_handlers_enabled via Dlsym before calling RegisterLibFunc, so older libchdb builds that don't export the signal handler control API (added in chdb-core#11) don't make this package's init() panic. When the symbol is missing: - the init's snapshot / set_enabled(0) / restore dance is skipped; - loadSigaction() is also skipped (libcSigaction stays nil); - the NewConnection guard becomes a no-op closure; - the two signal-handler tests t.Skip() instead of crashing. Net effect: on old libchdb the binding loads and works exactly as it did before this PR — the issue #30 fix simply doesn't apply. On v26.x+ the fix is active. The NewConnection call site now uses `defer guardSignalHandlers()()` which captures the snapshot at defer-statement time and returns either the restore closure or a no-op. Cleaner than the previous explicit saved/defer pair. Verified with a Dlsym probe that purego returns (handle, nil) for present symbols and (0, err) for missing ones on both macOS and Linux. Full `go test -race ./...` still green on macOS arm64 and Linux arm64.
1 parent ac20f3c commit 1b2f6ad

3 files changed

Lines changed: 71 additions & 34 deletions

File tree

‎chdb-purego/binding.go‎

Lines changed: 53 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -176,29 +176,58 @@ func init() {
176176
purego.RegisterLibFunc(&chdbResultStorageBytesRead, libchdb, "chdb_result_storage_bytes_read")
177177
purego.RegisterLibFunc(&chdbResultError, libchdb, "chdb_result_error")
178178

179-
purego.RegisterLibFunc(&chdbSetSignalHandlersEnabled, libchdb, "chdb_set_signal_handlers_enabled")
180-
purego.RegisterLibFunc(&chdbResetSignalHandlers, libchdb, "chdb_reset_signal_handlers")
181-
182-
// Tell libchdb NOT to install its own signal handlers on the first
183-
// chdb_connect(). Without this, libchdb's ClickHouse daemon code
184-
// installs handlers for SIGSEGV / SIGABRT / SIGBUS / SIGILL / SIGFPE
185-
// the first time a connection is opened, overwriting the handlers
186-
// the Go runtime relies on for stack growth and panic recovery. When
187-
// a Go signal then lands in libchdb's handler instead of the
188-
// runtime's, the result is the rare std::mutex::unlock crash
189-
// described in issue #30 — most often on macOS arm64 CI where signal
190-
// pressure is highest.
191-
//
192-
// Important: chdb_set_signal_handlers_enabled(0) doesn't only set a
193-
// flag — it also wipes the existing handlers in the same set back to
194-
// SIG_DFL (verified empirically; the chdb.h header doesn't mention
195-
// this side effect). Since the Go runtime has already installed its
196-
// handlers by the time this package's init runs, we have to save Go's
197-
// handlers, call the disable, then restore them. After this dance the
198-
// libchdb flag is set (so no future connect installs handlers) and
199-
// Go's handlers remain in place.
200-
loadSigaction()
179+
// Signal handler protection (issue #30). The required API was added in
180+
// libchdb v26.x via chdb-core#11. Pre-check the symbol with Dlsym so
181+
// that older libchdb builds — which don't export
182+
// chdb_set_signal_handlers_enabled — degrade gracefully instead of
183+
// panicking inside RegisterLibFunc at init. On those builds the crash
184+
// from issue #30 stays unfixed, but the binding still loads.
185+
if sym, dlsymErr := purego.Dlsym(libchdb, "chdb_set_signal_handlers_enabled"); dlsymErr == nil && sym != 0 {
186+
purego.RegisterLibFunc(&chdbSetSignalHandlersEnabled, libchdb, "chdb_set_signal_handlers_enabled")
187+
purego.RegisterLibFunc(&chdbResetSignalHandlers, libchdb, "chdb_reset_signal_handlers")
188+
189+
// Tell libchdb NOT to install its own signal handlers on the
190+
// first chdb_connect(). Without this, libchdb's ClickHouse
191+
// daemon code installs handlers for SIGSEGV / SIGABRT / SIGBUS
192+
// / SIGILL / SIGFPE the first time a connection is opened,
193+
// overwriting the handlers the Go runtime relies on for stack
194+
// growth and panic recovery. When a Go signal then lands in
195+
// libchdb's handler instead of the runtime's, the result is
196+
// the rare std::mutex::unlock crash described in issue #30 —
197+
// most often on macOS arm64 CI where signal pressure is
198+
// highest.
199+
//
200+
// Important: chdb_set_signal_handlers_enabled(0) doesn't only
201+
// set a flag — it also wipes the existing handlers in the same
202+
// set back to SIG_DFL (verified empirically; the chdb.h header
203+
// doesn't mention this side effect). Since the Go runtime has
204+
// already installed its handlers by the time this package's
205+
// init runs, we have to save Go's handlers, call the disable,
206+
// then restore them. After this dance the libchdb flag is set
207+
// (so no future connect installs handlers) and Go's handlers
208+
// remain in place.
209+
loadSigaction()
210+
saved := snapshotSignalHandlers()
211+
chdbSetSignalHandlersEnabled(0)
212+
restoreSignalHandlers(saved)
213+
}
214+
}
215+
216+
// signalProtectionAvailable reports whether the running libchdb exposes the
217+
// signal handler control API. When false, snapshotSignalHandlers and
218+
// restoreSignalHandlers must NOT be called (libcSigaction was not loaded).
219+
func signalProtectionAvailable() bool {
220+
return chdbSetSignalHandlersEnabled != nil
221+
}
222+
223+
// guardSignalHandlers returns a function suitable for `defer`-ing around a
224+
// call into libchdb. When signal protection is available, it captures the
225+
// current sigaction state now and the returned closure restores it. When
226+
// not available (old libchdb), both calls are no-ops.
227+
func guardSignalHandlers() func() {
228+
if !signalProtectionAvailable() {
229+
return func() {}
230+
}
201231
saved := snapshotSignalHandlers()
202-
chdbSetSignalHandlersEnabled(0)
203-
restoreSignalHandlers(saved)
232+
return func() { restoreSignalHandlers(saved) }
204233
}

‎chdb-purego/chdb.go‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -220,17 +220,19 @@ func NewConnection(argc int, argv []string) (ChdbConn, error) {
220220
// chdb_connect — even after chdb_set_signal_handlers_enabled(0) —
221221
// still resets the kernel sigaction table for SIGSEGV / SIGABRT /
222222
// SIGBUS / SIGILL / SIGFPE to SIG_DFL on the first call (verified
223-
// empirically; see issue #30 for context). Snapshot Go's handlers
224-
// and restore them on return so the runtime keeps its stack-growth
225-
// / panic-recovery handlers.
223+
// empirically; see issue #30 for context). guardSignalHandlers
224+
// snapshots Go's handlers now and the deferred closure restores
225+
// them on return — even if chdb_connect panics (a C++ exception
226+
// propagated through purego), the LIFO defer order runs
227+
// recover() first to set err and then the restore, leaving the
228+
// process with Go's stack-growth / panic-recovery handlers
229+
// intact.
226230
//
227-
// Restore is deferred BEFORE the panic-recovery defer so that even
228-
// if chdb_connect panics (e.g. C++ exception propagated through
229-
// purego), Go's handlers are still put back before this function
230-
// unwinds — otherwise the rest of the process would be left with
231-
// SIG_DFL for these signals.
232-
saved := snapshotSignalHandlers()
233-
defer restoreSignalHandlers(saved)
231+
// On old libchdb builds that don't export the signal-handler
232+
// control API, guardSignalHandlers returns a no-op closure and
233+
// the call site behaves exactly as it did before the issue #30
234+
// fix landed.
235+
defer guardSignalHandlers()()
234236

235237
defer func() {
236238
if r := recover(); r != nil {

‎chdb-purego/stress_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ import (
2121
// breaks Go's stack growth and panic recovery; under load it surfaces as the
2222
// rare std::mutex::unlock crash reported on macOS arm64.
2323
func TestSignalHandlersPreservedAcrossConnect(t *testing.T) {
24+
if !signalProtectionAvailable() {
25+
t.Skip("libchdb does not export chdb_set_signal_handlers_enabled; protection disabled on this build")
26+
}
2427
before := snapshotSignalHandlers()
2528

2629
conn, err := NewConnectionFromConnString(":memory:")
@@ -57,6 +60,9 @@ func TestSignalHandlersPreservedAcrossConnect(t *testing.T) {
5760
// makes the restore unconditional. This test wipes a handler, panics, and
5861
// asserts that on unwinding the handler is back to what we snapshotted.
5962
func TestSignalHandlersRestoredAfterPanic(t *testing.T) {
63+
if !signalProtectionAvailable() {
64+
t.Skip("libchdb does not export chdb_set_signal_handlers_enabled; protection disabled on this build")
65+
}
6066
initial := snapshotSignalHandlers()
6167

6268
recovered := false

0 commit comments

Comments
 (0)