serramatutu commented on code in PR #3905:
URL: https://github.com/apache/arrow-adbc/pull/3905#discussion_r3720093973
##########
rust/driver_manager/src/lib.rs:
##########
@@ -565,6 +565,31 @@ pub struct ManagedConnection {
inner: Arc<ManagedConnectionInner>,
}
+struct ConnectionCancelHandle {
+ inner: std::sync::Weak<ManagedConnectionInner>,
+}
+
+impl adbc_core::CancelHandle for ConnectionCancelHandle {
+ fn try_cancel(&self) -> Result<()> {
+ if let Some(inner) = self.inner.upgrade() {
+ if let AdbcVersion::V100 = inner.database.driver.version {
+ return Err(Error::with_message_and_status(
+ ERR_CANCEL_UNSUPPORTED,
+ Status::NotImplemented,
+ ));
+ }
+ let driver = &inner.database.driver.driver;
+ let mut connection = inner.connection.lock().unwrap();
Review Comment:
`.lock().unwrap()` adds an expectation that the thread who owns the
`adbc_ffi::FFI_AdbcConnection` cannot have panicked (mutex poison) otherwise
trying to cancel any statements on that connection will also panic the canceler
thread, right? This would cause the panic to start spreading.
Instead of `unwrap()`, could we instead make this return an error so
canceler threads can deal with it gracefully?
Since the method is called `try_cancel()` I'd expect it to not panic if
canceling failed for any reason, including a panic in a neighbor thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
