feat!(rust/ffi): catch panics at FFI boundary #3819
Conversation
lidavidm
force-pushed
the
catch-panic
branch
7 times, most recently
from
e1f661e to
a2d7fc3
Compare
|
also CC @eitsupi if you're interested |
amoeba
left a comment
amoeba
left a comment
There was a problem hiding this comment.
Really nice. I hadn't seen the old behavior so I disabled it and re-ran the new tests and this PR is a big improvement. I had one question and a really tiny nit if you agree.
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn AdbcDriverInit( |
There was a problem hiding this comment.
As a backstop so that the entrypoint can be found. Otherwise you have to declare it manually which is a bit inconvenient.
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
Great!
I found some points to be improved.
rust/ffi/src/driver_exporter.rs
Outdated
| /// # Parameters | ||
| /// | ||
| /// - `$func_name` - Driver's initialization function name. The recommended name | ||
| /// is `AdbcDriverInit`, or a name derived from the name of the driver's shared |
There was a problem hiding this comment.
Perhaps the documentation needs to be updated to state that "AdbcDriverInit is a reserved name" (no longer be able to used after this change), and perhaps the PR should be renamed to feat(rust/ffi)! ... to make it clear that it is a breaking change.
There was a problem hiding this comment.
Ah, I should update that comment...the name should be the latter (a driver should provide both)
lidavidm
changed the title
|
I think 22b954d needs a slight fix which I'll look at |
This gives a slightly nicer experience than unconditionally crashing the entire program. As with the Go FFI scaffolding, once a driver panics, all further calls to the driver will fail. (Albeit, this is not true for exported readers. We also need our own C Data Interface export shim so we can handle things like exporting ADBC errors through the C Data Interface boundary. Currently, C++ and Go-based drivers can do this.)
Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
Co-authored-by: Bryce Mecum <petridish@gmail.com>
Co-authored-by: Bryce Mecum <petridish@gmail.com>
lidavidm
force-pushed
the
catch-panic
branch
from
f41b35f to
e3801d0
Compare
3 participants
This gives a slightly nicer experience than unconditionally crashing the entire program. As with the Go FFI scaffolding, once a driver panics, all further calls to the driver will fail. (Albeit, this is not true for exported readers. We also need our own C Data Interface export shim so we can handle things like exporting ADBC errors through the C Data Interface boundary. Currently, C++ and Go-based drivers can do this.)