spsc: fix split lifetimes - #29
matoushybl wants to merge 6 commits into
Conversation
493be9d to
6091af6
Compare
6091af6 to
72319c5
Compare
The only code changes were: - Marking a now-unsafe attribute as unsafe - Removing the old Captures hack. This bumps the MSRV for the edition support.
In the current implementation, it was not possible to split the queue in a function where the queue was passed as a mutable reference. For example when wrapping Pusher and Popper in custom types. This failed with an "`queue` dropped here while still borrowed, borrow might be used here, when `queue` is dropped and runs the `Drop` code for type `spsc::Queue`". This commit attempts to solve it by introducing an explicit lifetime of the &mut capture when splitting, which is shorter than the lifetime of the underlying data. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=17cad726a74a1be401924d2686e757ab
72319c5 to
1d8330e
Compare
There was a problem hiding this comment.
Hey, sorry I missed this initially. This is a good catch.
The tests in the PR don't actually build, but I'm fixing them and I'll push the fixes to your branch.
This is a major breaking change and would require a 2.0.0 lilos. However, the original API here seems broken, since it was explicitly intended to allow split in this case, and it didn't (due to the problem you've found). So I'm kind of inclined to start preparing for a 2.0 release, but it may have to wait a bit.
(Edit: See my comment below, I'm not sure the API change is necessary to unstick this use case.)
| fn compile_test_split_lifetime() { | ||
| fn split<'split, 'storage>( | ||
| queue: &mut Queue<'storage, ()>, | ||
| ) -> (Pusher<'storage, ()>, Popper<'storage, ()>) { |
There was a problem hiding this comment.
The 'split lifetime is missing from Pusher/Popper here, so the test doesn't compile (I'm fixing this)
| queue.split() | ||
| } | ||
|
|
||
| let mut storage: [MaybeUninit<u8>; 5] = [MaybeUninit::uninit(); 5]; |
There was a problem hiding this comment.
u8 should be () here for consistency with the function signature above (fixing this)
|
I've gotten the code in your playground example to work, and I think it comes down to the signature of the You had: fn split<'storage>(q: &'storage mut Queue<'storage>) -> (Pusher<'storage>, Popper<'storage>) {This constrains the queue's lifetime to match the storage's lifetime. It's actually over-constrained compared to lilos's split routine, which is (with an implicit lifetime filled in for clarity): impl<'storage> Queue<'storage> {
pub fn split(&'q mut self) -> (Pusher<'q, T>, Popper<'q, T>);
}or as a standalone function, that would be equivalent to fn split<'q, 'storage, T>(&'q mut Queue<'storage, T>) -> (Pusher<'q, T>, Popper<'q, T>);This avoids connecting the lifetime of the queue itself ( Here is a modified version of your playground example where I've rewritten the fn split<'q>(q: &'q mut Queue<'_>) -> (Pusher<'q>, Popper<'q>) {
q.split()
}This works. I know it's been a bit since you filed this issue, but if you still have the code you were writing that motivated this PR, can you try rewriting any routines that split queues using this technique and let me know if it works? |
3 participants
In the current implementation, it was not possible to split the queue in a function where the queue was passed as a mutable reference. For example when wrapping Pusher and Popper in custom types. This failed with an "
queuedropped here while still borrowed, borrow might be used here, whenqueueis dropped and runs theDropcode for typespsc::Queue". This commit attempts to solve it by introducing an explicit lifetime of the &mut capture when splitting, which is shorter than the lifetime of the underlying data.https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=17cad726a74a1be401924d2686e757ab