feat(rust): define cancellation in a sensible way - #3905
Conversation
lidavidm
changed the title
|
Any concerns here? |
|
I started looking at the code and got confused. Are you going to cancel any statement currently running in the connection or do I get one handle per |
|
One per statement (the handle has a weak reference to the specific statement) |
abonander
left a comment
abonander
left a comment
There was a problem hiding this comment.
This seems all right, the inheritance can be implemented using tokio_util::sync::CancellationToken::child_token().
Just a few questions about the detailed semantics.
| /// Cancel the in-progress operation on a connection. | ||
| fn try_cancel(&self) -> Result<()>; |
There was a problem hiding this comment.
Are the try_ prefix and the default to NoOpCancellationHandle meant to imply that this is on a best-effort basis only? Can that be more clearly documented?
There was a problem hiding this comment.
It's only sort-of implied by
arrow-adbc/c/include/arrow-adbc/adbc.h
Lines 2130 to 2131 in e8642f8
| /// | ||
| /// This is a separated handle because otherwise it would be impossible to | ||
| /// call a `cancel` method on a connection or statement itself. | ||
| pub trait CancelHandle: Send { |
There was a problem hiding this comment.
arrow-adbc/c/include/arrow-adbc/adbc.h
Lines 2133 to 2134 in e8642f8
"thread-safe" implies that this handle should be Sync as well since there isn't really a distinction between "can be moved between threads" and "can be called concurrently from multiple threads" in C.
|
|
||
| impl CancelHandle for NoOpCancellationHandle { | ||
| fn try_cancel(&self) -> Result<()> { | ||
| Ok(()) |
There was a problem hiding this comment.
Should the default be to return ADBC_STATUS_UNKNOWN to indicate that the operation cannot be cancelled per
arrow-adbc/c/include/arrow-adbc/adbc.h
Line 2143 in e8642f8
| fn get_cancel_handle(&self) -> Box<dyn CancelHandle> { | ||
| Box::new(NoOpCancellationHandle {}) | ||
| } |
There was a problem hiding this comment.
Are you going to introduce AdbcDatabaseCancel() to the C API? Otherwise I don't see why this needs to exist.
| )); | ||
| } | ||
| let driver = &inner.database.driver.driver; | ||
| let mut connection = inner.connection.lock().unwrap(); |
There was a problem hiding this 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.
| )); | ||
| } | ||
| let driver = &inner.connection.database.driver.driver; | ||
| let mut statement = inner.statement.lock().unwrap(); |
|
Changes:
|
lidavidm
changed the title
4 participants
Closes #3454.