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.
There was a problem hiding this comment.
Right, but isn't 'static here requiring that the handler has a static lifetime? Why not use the lifetime of self?
There was a problem hiding this comment.
Ideally it should be lifetime of self. But Box<ConnectionCallback> cannot be annotated with lifetime so it does not compile. Maybe it is possible somehow or maybe more unsafe trick is needed. It can be a future improvement.
| @@ -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?
There was a problem hiding this comment.
The test executes this path and it seems to work, but I think this is UB, the test runs but there is no guarantee from compiler this will work in the future, or under certain optimizations.
We can document that closing handle inside the callback is not safely supported in rust, and user wishing to do so needs to do proper testing for the UB, or use unsafe to extract the ctx and clean it up outside the callback.
In practice closing the handle in the callback might not be a prevalent use case.
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
| @@ -804,30 +877,64 @@ impl Default for Connection { | |||
| } | |||
| } | |||
|
|
|||
| // Remarks on Rust callback in general: | |||
| // Callback function or closure is set to msquic handle context of type `*mut Box<TCallback>>`. | |||
| // We cannot use type `*mut TCallback` because dyn trait cannot be converted to a pointer directly. | |||
There was a problem hiding this comment.
Issue here being that &dyn Trait is implemented as a 'fat pointer' with a pointer to both the underlying impl along with the vtable for that impl's implementation of the trait. So a single 8 byte pointer won't hold the 16 byte 'fat pointer'
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