Commit54edad2

and

committed

Add rapi_connection_valid C++ entry point to avoid deadlock in dbIsValid() calls from progress bar handlers

Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>

1 parentd35bdab commit54edad2Copy full SHA for 54edad2

File tree

+41

-10

lines changed

+41

-10

lines changed

Lines changed: 4 additions & 10 deletions

Original file line number	Diff line number	Diff line change
`@@ -2,16 +2,10 @@`
`2`	`2`	`#' @inheritParams DBI::dbIsValid`
`3`	`3`	`#' @usage NULL`
`4`	`4`	`dbIsValid__duckdb_connection<-function(dbObj,...) {`
`5`		`-valid<-FALSE`
`6`		`- tryCatch(`
`7`		`- {`
`8`		`- dbGetQuery(dbObj, SQL("SELECT 1"))`
`9`		`-valid<-TRUE`
`10`		`- },`
`11`		`-error=function(c) {`
`12`		`- }`
`13`		`- )`
`14`		`-valid`
	`5`	`+# Use fast C++ connection validity check instead of executing SELECT 1`
	`6`	`+# This avoids deadlock issues when called from progress bar handlers`
	`7`	`+# that already have ScopedInterruptHandler protection`
	`8`	`+ .Call(rapi_connection_valid,dbObj@conn)`
`15`	`9`	`}`
`16`	`10`
`17`	`11`	`#' @rdname duckdb_connection-class`

Lines changed: 28 additions & 0 deletions

Original file line number	Diff line number	Diff line change
`@@ -84,3 +84,31 @@ static void SetDefaultConfigArguments(ClientContext &context) {`
`84`	`84`	`delete conn_wrapper;`
`85`	`85`	`}`
`86`	`86`	`}`
	`87`	`+`
	`88`	`+[[cpp11::register]]boolrapi_connection_valid(duckdb::conn_eptr_t conn) {`
	`89`	`+// Check connection validity without acquiring ClientContext locks`
	`90`	`+// This avoids the "ScopedInterruptHandler already active" issue when`
	`91`	`+// called from progress bar handlers or other contexts that already`
	`92`	`+// have interrupt protection`
	`93`	`+if (!conn \|\| !conn.get()) {`
	`94`	`+returnfalse;`
	`95`	`+}`
	`96`	`+`
	`97`	`+auto conn_wrapper = conn.get();`
	`98`	`+if (!conn_wrapper) {`
	`99`	`+returnfalse;`
	`100`	`+}`
	`101`	`+`
	`102`	`+// Check if the connection object exists`
	`103`	`+if (!conn_wrapper->conn) {`
	`104`	`+returnfalse;`
	`105`	`+}`
	`106`	`+`
	`107`	`+// Check if the database wrapper is still valid`
	`108`	`+if (!conn_wrapper->db \|\| !conn_wrapper->db->db) {`
	`109`	`+returnfalse;`
	`110`	`+}`
	`111`	`+`
	`112`	`+// All checks passed - connection appears valid`
	`113`	`+returntrue;`
	`114`	`+}`

Lines changed: 2 additions & 0 deletions

Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,8 @@ duckdb::conn_eptr_t rapi_connect(duckdb::db_eptr_t);`
`219`	`219`
`220`	`220`	`voidrapi_disconnect(duckdb::conn_eptr_t);`
`221`	`221`
	`222`	`+boolrapi_connection_valid(duckdb::conn_eptr_t);`
	`223`	`+`
`222`	`224`	`cpp11::listrapi_prepare(duckdb::conn_eptr_t, std::string);`
`223`	`225`
`224`	`226`	`cpp11::listrapi_bind(duckdb::stmt_eptr_t, SEXP paramsexp, duckdb::ConvertOpts);`

Lines changed: 7 additions & 0 deletions

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,13 @@`
`9`	`9`
`10`	`10`	`namespaceduckdb {`
`11`	`11`
	`12`	`+//! ScopedInterruptHandler serves a dual purpose:`
	`13`	`+//! 1. Install interrupt handler: Installs a signal handler to catch SIGINT`
	`14`	`+//! and properly interrupt DuckDB operations by calling ClientContext::Interrupt()`
	`15`	`+//! 2. Avoid reentrant calls: Ensures only one ScopedInterruptHandler is active at a time,`
	`16`	`+//! preventing deadlocks when operations are called from within signal handlers`
	`17`	`+//! (such as progress bar callbacks). Throws "ScopedInterruptHandler already active"`
	`18`	`+//! if instantiated while another instance is already active.`
`12`	`19`	`classScopedInterruptHandler {`
`13`	`20`	`private:`
`14`	`21`	`shared_ptr<ClientContext> context;`

Comments

(0)