From f5e4645fbe7fd13f773c91dc186eae0260a789c2 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 4 Jun 2025 18:06:23 +0200 Subject: [PATCH 1/2] Update some comments to note why we need StaticRawIter/SendPtr wrapper types Unfortunately as we are holding NonNulls across await points, we'll still need some kind of adapter to mark this as sound. This means that https://github.com/hawkw/mycelium/pull/536 is not in and of itself sufficient. Comments have been updated to reflect this. --- src/intrusive.rs | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/intrusive.rs b/src/intrusive.rs index 8cb2175..4b91b23 100644 --- a/src/intrusive.rs +++ b/src/intrusive.rs @@ -815,7 +815,7 @@ async fn verify_list_in_flash( let mut queue_iter = flash.iter().await.map_err(LoadStoreError::FlashRead)?; // Make it Send - let mut iter = StaticRawIter { + let iter = StaticRawIter { iter: ls.iter_raw(), }; @@ -824,7 +824,7 @@ async fn verify_list_in_flash( let mut counter_found = false; // Iterate over the nodes in the list - while let Some(hdrptr) = iter.next() { + for hdrptr in iter { let header = unsafe { hdrptr.ptr.as_ref() }; // TODO @James: Can you think of an easier way of doing things here? @@ -890,11 +890,11 @@ async fn write_to_flash( buf: &mut [u8], flash: &mut Flash, ) -> Result<(), LoadStoreError> { - let mut iter = StaticRawIter { + let iter = StaticRawIter { iter: ls.iter_raw(), }; - while let Some(hdrptr) = iter.next() { + for hdrptr in iter { // Attempt to serialize let res = serialize_node(hdrptr.ptr, buf); @@ -1325,16 +1325,27 @@ where // Helper structs and functions // -------------------------------------------------------------------------- -/// Wrapper for [`IterRaw`] that implements `Send` +/// Wrapper for [`IterRaw`] that implements `Send`. /// -/// This is only a helper struct until this issue is resolved: -/// +/// This WOULD be addressed by https://github.com/hawkw/mycelium/pull/536, which +/// makes IterRaw impl Send, however we still need to wrap the yielded NonNulls, +/// which are not Send. Therefore, we will keep this structure mostly for the +/// ability to wrap the Iterator impl to return Send-implementing [`SendPtr`]s +/// instead of `NonNull`s. struct StaticRawIter<'a> { iter: IterRaw<'a, NodeHeader>, } + +/// ## Safety +/// The contained IterRaw is only valid for the lifetime of the List it comes +/// from, which can only be obtained by holding the mutex. This means that we +/// have exclusive access, and all nodes must be 'static. Therefore, it is +/// sound to Send both the iterator, and the wrapped NonNulls it returns. unsafe impl Send for StaticRawIter<'_> {} -impl StaticRawIter<'_> { +impl Iterator for StaticRawIter<'_> { + type Item = SendPtr; + fn next(&mut self) -> Option { self.iter.next().map(|ptr| SendPtr { ptr }) } @@ -1342,10 +1353,15 @@ impl StaticRawIter<'_> { /// Wrapper for [`NonNull`] that implements `Send`. /// +/// This is necessary because we iterate over the IterRaw in async context, +/// and for testing this means that futures need to be send. Since the IterRaw +/// yields `NonNull`s, the iterated nodes are not Send. This adapter is +/// sound because for as long as we have the IterRaw live, the mutex must remain +/// locked. +/// /// ## Safety /// This must only be used when the List mutex is locked and Node and Anchor /// live &'static. -/// TODO: Remove this once https://github.com/hawkw/mycelium/pull/536 is merged. struct SendPtr { ptr: NonNull, } From cfaa528c60e7ca6c0f5c49e7284fc2ce41554f07 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 4 Jun 2025 20:21:12 +0200 Subject: [PATCH 2/2] Update src/intrusive.rs Co-authored-by: Julian <20155974+JuliDi@users.noreply.github.com> --- src/intrusive.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/intrusive.rs b/src/intrusive.rs index 4b91b23..3c2b6e0 100644 --- a/src/intrusive.rs +++ b/src/intrusive.rs @@ -1354,7 +1354,7 @@ impl Iterator for StaticRawIter<'_> { /// Wrapper for [`NonNull`] that implements `Send`. /// /// This is necessary because we iterate over the IterRaw in async context, -/// and for testing this means that futures need to be send. Since the IterRaw +/// and for testing this means that futures need to be Send. Since the IterRaw /// yields `NonNull`s, the iterated nodes are not Send. This adapter is /// sound because for as long as we have the IterRaw live, the mutex must remain /// locked.