Add Bulk transfer async API#64
Conversation
TheButlah
changed the title
| pub fn new_bulk( | ||
| device: &'d DeviceHandle<C>, | ||
| endpoint: u8, | ||
| buffer: &'b mut [u8], |
There was a problem hiding this comment.
If you mem::forget() the AsyncTransfer so its drop never runs to cancel the transfer, this buffer may be invalidated or mutably borrowed elsewhere while libusb is still handling a transfer accessing it.
Unfortunately Pin cannot solve this. It's the same problem that Rust APIs for io_uring run into. The transfer needs to own the buffer so that it remains valid in case drop is not run.
| /// returned Err, or the callback has gotten an Err. | ||
| // Step 3 of async API | ||
| #[allow(unused)] | ||
| pub fn submit(&mut self) -> Result<(), TransferError> { |
There was a problem hiding this comment.
Can you even call this with &mut self when the constructor returns it as Pin? If you could obtain a &mut AsyncTransfer without unsafe, then you can also mem::swap it, and your use of Pin wouldn't be doing any good in protecting its self-reference.
There was a problem hiding this comment.
Yeah, you're right. I put it as mut because I wasnt sure if it would be safe to have it as a shared reference. Can I just change it to &? Would I need to make it !Sync?
There was a problem hiding this comment.
Or perhaps it should be submit(self: &mut Pin<Box<Self>>) ?
There was a problem hiding this comment.
&self may actually be safe given the locking inside libusb_submit_transfer, but I'd have to look more closely.
| // inside the custom user data field of the transfer struct, and then call the | ||
| // user provided closure from there. | ||
| // Step 4 of async API | ||
| extern "system" fn transfer_cb(transfer: *mut ffi::libusb_transfer) { |
There was a problem hiding this comment.
If the user-supplied callback panics, it's undefined behavior for it to propagate into C, so this needs a catch_unwind.
| // Safety: The pointer shouldn't be a fat pointer, and should be valid, so | ||
| // this should be sound | ||
| let closure = unsafe { | ||
| let closure: *mut F = std::mem::transmute(transfer.user_data); | ||
| &mut *closure | ||
| }; |
There was a problem hiding this comment.
libusb_cancel_transfer in the drop implementation requests cancellation asynchronously, but after drop returns, the AsyncTransfer memory may be invalidated before this callback is guaranteed to run (or complete, if already running on another thread). The callback may even still be called with a transfer completion after cancellation if it was in the middle of completing at the time of cancellation. Therefore this reference is not guaranteed to be valid.
There was a problem hiding this comment.
The callback may even still be called with a transfer completion after cancellation if it was in the middle of completing at the time of cancellation.
Are you saying this ordering is possible:
- Submit transfer
- Drop
- Callback informs of cancellation
- Callback informs of transfer completion
I wouldn't expect 4 to ever come before 3 - are you sure that is possible? If not possible, perhaps I could actually intentionally leak the struct with Box::leak, and then get the box back inside the cancellation callback and properly drop the components?
There was a problem hiding this comment.
The callback should only get called once for a single transfer (until resubmitted), so you won't get both 3 and 4. What I'm not sure about is if you can get a LIBUSB_TRANSFER_COMPLETED callback after calling libusb_transfer_cancel if the transfer has already completed but the event loop has not called the callback yet, or whether this would still be LIBUSB_TRANSFER_CANCELLED but have nonzero actual_length.
I think deferring the drop to the callback could work, though I'd probably use ManuallyDrop over Box::leak to make it explicit.
| match transfer.status { | ||
| LIBUSB_TRANSFER_CANCELLED => { | ||
| // Step 5 of async API: Transfer was cancelled, free the transfer | ||
| unsafe { ffi::libusb_free_transfer(transfer) } |
There was a problem hiding this comment.
This is the only call to libusb_free_transfer. If the transfer has already completed on drop, the callback is never called with this status and the libusb_transfer is leaked.
| LIBUSB_TRANSFER_COMPLETED => { | ||
| debug_assert!(transfer.length >= transfer.actual_length); // sanity | ||
| let data = unsafe { | ||
| std::slice::from_raw_parts(transfer.buffer, transfer.actual_length as usize) |
There was a problem hiding this comment.
One other safety consideration is that the transfer is no longer considered busy by libusb at the time of the callback, and the AsyncTransfer might be owned by another thread that could resubmit it during the execution of this callback or the user closure. That would illegally alias and mutate the transfer and buffer references here.
|
This also inspired me to finally write up some thoughts on async USB in Rust in general and an API for it I've been planning on implementing but haven't gotten to yet. Safety issues aside, callbacks are awkward in Rust due to single ownership, and would require a bunch of Posted more over at #62 (comment) to avoid hijacking your PR. |
|
Closing in favor of #65 |
2 participants
Closes #62
Disclaimer: I am very new to unsafe rust, self-referential structs, and libusb's async API. Please vet this code.
This WIP PR adds support for bulk reads with libusb's async API. The following needs to be done before a merge: