Skip to content

fix(rust/driver_manager): allow cancel to be truly concurrent - #4858

Merged
lidavidm merged 1 commit into
apache:mainfrom
lidavidm:gh-4817
Oct 8, 2026
Merged

lidavidm merged 1 commit into
apache:mainfrom
lidavidm:gh-4817

Conversation

@lidavidm

@lidavidm lidavidm commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

N.B. the driver exporter is not covered by this PR.

Closes #4817.

Assisted-by: GPT-6 Astra

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lidavidm
lidavidm marked this pull request as ready for review October 6, 2026 04:06
@lidavidm
lidavidm requested a review from wjones127 as a code owner October 6, 2026 04:06
@lidavidm

lidavidm commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

JavaScript failures are unrelated, I will address separately.

lidavidm added a commit to lidavidm/arrow-adbc that referenced this pull request Oct 6, 2026

@amoeba amoeba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I can't say I'm expert enough in ways of handling this but the bug looks real and the test looks good.

Comment thread rust/driver_manager/src/lib.rs Outdated
Comment on lines 98 to 104
// 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: re-hard-wrap this. Not sure if this will work right (I did it from the github UI)

Suggested change
// 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.

@lidavidm
lidavidm merged commit a43f0fe into apache:main Oct 8, 2026
22 checks passed
@lidavidm
lidavidm deleted the gh-4817 branch October 8, 2026 22:39
lidavidm added a commit to lidavidm/arrow-adbc that referenced this pull request Oct 8, 2026
lidavidm added a commit to lidavidm/arrow-adbc that referenced this pull request Oct 8, 2026
lidavidm added a commit to lidavidm/arrow-adbc that referenced this pull request Oct 8, 2026
lidavidm added a commit to lidavidm/arrow-adbc that referenced this pull request Oct 9, 2026
lidavidm added a commit that referenced this pull request Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(rust/driver_manager): cancellation waits for the operation it should interrupt

4 participants