Repository navigation
fix(rust/driver_manager): allow cancel to be truly concurrent - #4858
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The new unsafe synchronization model permits concurrent native FFI access and warrants final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Enables concurrent cancellation in the Rust driver manager while preserving serialized access for ordinary operations.
Changes:
- Adds raw-handle option setters for managed FFI access.
- Introduces lifetime-safe native handle synchronization.
- Adds cancellation concurrency, lifetime, error, and compatibility tests.
| File | Description |
|---|---|
rust/ffi/src/options.rs |
Adds unsafe raw-handle option setters. |
rust/driver_manager/src/lib.rs |
Allows cancellation to bypass ordinary-operation locks safely. |
rust/driver_manager/tests/cancellation.rs |
Tests concurrency, handle lifetimes, errors, and unsupported cancellation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
JavaScript failures are unrelated, I will address separately. |
Depends on apache#4858. Closes apache#4857.
amoeba
left a comment
There was a problem hiding this comment.
+1. I can't say I'm expert enough in ways of handling this but the bug looks real and the test looks good.
| // Databases use a `Mutex`. Connections and statements use `NativeHandle` to | ||
| // serialize ordinary calls while allowing cancellation through raw pointers. | ||
| // Their shared inner objects release the native handles only after all calls | ||
| // (including cancellation) have relinquished their strong references. | ||
| // `FFI_Driver` is not wrapped in a `Mutex` because it is | ||
| // an immutable struct of function pointers. Wrapping the driver in a `Mutex` | ||
| // would prevent any parallelism between driver calls, which is not desirable. |
There was a problem hiding this comment.
nit: re-hard-wrap this. Not sure if this will work right (I did it from the github UI)
| // Databases use a `Mutex`. Connections and statements use `NativeHandle` to | |
| // serialize ordinary calls while allowing cancellation through raw pointers. | |
| // Their shared inner objects release the native handles only after all calls | |
| // (including cancellation) have relinquished their strong references. | |
| // `FFI_Driver` is not wrapped in a `Mutex` because it is | |
| // an immutable struct of function pointers. Wrapping the driver in a `Mutex` | |
| // would prevent any parallelism between driver calls, which is not desirable. | |
| // Databases use a `Mutex`. Connections and statements use `NativeHandle` to | |
| // serialize ordinary calls while allowing cancellation through raw pointers. | |
| // Their shared inner objects release the native handles only after all calls | |
| // (including cancellation) have relinquished their strong references. | |
| // `FFI_Driver` is not wrapped in a `Mutex` because it is an immutable struct of | |
| // function pointers. Wrapping the driver in a `Mutex` would prevent any | |
| // parallelism between driver calls, which is not desirable. |
Assisted-by: GPT-6 Astra
Depends on apache#4858. Closes apache#4857.
Depends on apache#4858. Closes apache#4857.
Depends on apache#4858. Closes apache#4857.
Depends on apache#4858. Closes apache#4857.
N.B. the driver exporter is not covered by this PR.
Closes #4817.
Assisted-by: GPT-6 Astra