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
connection cannot have panicked (mutex poison) otherwise trying to cancel any
statements on that connection will also panic the canceler thread, right?
Instead of `unwrap()`, could we instead make this return an error so that a
panic in the owning thread doesn't spread to canceler threads and they can deal
with it gracefully?
Also since the method is called `try_cancel()` I'd expect it to not panic if
canceling failed for any reason.
--
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]
