Rust: Safe Api for callbacks #4882
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4882 +/- ##
==========================================
- Coverage 87.55% 86.96% -0.59%
==========================================
Files 56 56
Lines 17726 17726
==========================================
- Hits 15520 15416 -104
- Misses 2206 2310 +104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) -> Result<(), Status> { | ||
| pub fn open<F>(&mut self, registration: &Registration, handler: F) -> Result<(), Status> | ||
| where | ||
| F: FnMut(ConnectionRef, ConnectionEvent) -> Result<(), Status> + 'static, |
There was a problem hiding this comment.
Isn't the lifetime of the connection enough here? Callbacks will likely have a context with a limited lifetime.
There was a problem hiding this comment.
Callback closure has the lifetime as the handle.
It should be dropped after handle is closed.
| Some(test_conn_callback), | ||
| &connection as *const Connection as *const c_void, | ||
| ); | ||
| let res = connection.open(®istration, test_conn_callback); |
There was a problem hiding this comment.
It would be interesting to keep a test that provide some context (since any non-trivial application would need to), to make sure it is correctly handled through Rust lifetime validation + unsafe code. A counter for the number of calls could be an easy one.
There was a problem hiding this comment.
I have a full server client test in another branch. Will make it to main soon.
There was a problem hiding this comment.
Added the full server client test in this PR.
| @@ -1423,7 +1561,7 @@ mod tests { | |||
| } => { | |||
| println!("Stream shutdown complete: {connection_shutdown} {app_close_in_progress} {connection_shutdown_by_app} {connection_closed_remotely} {connection_error_code} {connection_close_status}"); | |||
| // Attach to stream for auto close handle. | |||
| unsafe { Stream::from_raw(stream) }; | |||
| unsafe { Stream::from_raw(stream.as_raw()) }; | |||
There was a problem hiding this comment.
The only uncertainty I have for this PR is this line.
Ctx is dropped at this line, but the code of execution is inside the callback. This means the current function/closure has already been dropped after this line, and the function has not returned yet. Is this UB?
But naively thinking, after this line if code does not access any closure variables (already dropped), the code is ok. Because the stack is still present, and the remaining function call instruction is just propagating return value to upper stack frame. So far valgrind does not find any memory use problem.
But I still worry this is UB (undefined behaviour).
There was a problem hiding this comment.
So, how do we resolve this uncertainty? Can we test it?
youyuanwu
force-pushed
the
users/youyuanwu/ffi-fn-callback
branch
from
75b0dd4 to
6df357d
Compare
* rebase test to fn branch * enable server client tests on windows * enable manual trigger for cargo ci
Description
Use Rust Fn and FnMut for user callback function or closure, so user no longer need to use unsafe extern C function and managing callback context ptr.
Added macro to define common HandleRef type, and set get context functions to reduce code duplication.
Testing
Modified the existing test callbacks to use the new APIs.
Documentation
NA