Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix AbiExample for Arc/Rc's Weaks
  • Loading branch information
ryoqun committed Oct 4, 2023
commit f5bdb30fe9d52baa95190a38e2f10252534a65d6
8 changes: 8 additions & 0 deletions frozen-abi/src/abi_digester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,4 +691,12 @@ mod tests {
Variant2(u8, u16, #[serde(skip)] u32),
}
}

#[frozen_abi(digest = "B1PcwZdUfGnxaRid9e6ZwkST3NZ2KUEYobA1DkxWrYLP")]
#[derive(Serialize, AbiExample)]
struct TestArcWeak(std::sync::Weak<u64>);

#[frozen_abi(digest = "4R8uCLR1BVU1aFgkSaNyKcFD1FeM6rGdsjbJBFpnqx4v")]
#[derive(Serialize, AbiExample)]
struct TestRcWeak(std::rc::Weak<u64>);
}
22 changes: 20 additions & 2 deletions frozen-abi/src/abi_example.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you can see, enabling frozen_abi for banking trace files wasn't so joyful... xD

Original file line number Diff line number Diff line change
Expand Up @@ -316,10 +316,20 @@ impl<T: AbiExample> AbiExample for std::sync::Arc<T> {
}
}

// When T is weakly owned by the likes of `std::{sync, rc}::Weak`s, we need to uphold the ownership
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the explanation here.
I think this makes sense because if we try serializing a Weak it will upgrade, and always return None without this leakage.

However, I'm not super clear on why we need to have the implementations for Weak at all.
The struct I see it used, which has now become AbiExample, is PinnedVec; but on the PinnedVec we have:

    #[serde(skip)]
    recycler: Weak<RecyclerX<PinnedVec<T>>>,

so shouldn't this Weak be ignored in any serialization? Does that not include the digests we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I'm not super clear on why we need to have the implementations for Weak at all.
...

another good question. yeah, some unspoken nuances are lying here... ;) i did my best to answer that in patch: fd6c28d

I think this makes sense because if we try serializing a Weak it will upgrade, and always return None without this leakage.

yep, i noticed this bug in Weak::example() later after creating this pr and this commit specifically fixed: f5bdb30

as you correctly pointed out, Weak isn't used for actual abi digesting currently. but for perfection, i did the extra effort by adding unit test as well, should we serialize Weak in distant future... ;)

// of T in some way at least during abi digesting... However, there's no easy way. Stashing them
// into static is confronted with Send/Sync issue. Stashing them into thread_local is confronted
// with not enough (T + 'static) lifetime bound.. So, just leak the examples. This should be
// tolerated, considering ::example() should ever be called inside tests, not in production code...
fn leak_and_inhibit_drop<'a, T>(t: T) -> &'a mut T {
Box::leak(Box::new(t))
}

impl<T: AbiExample> AbiExample for std::sync::Weak<T> {
fn example() -> Self {
info!("AbiExample for (Weak<T>): {}", type_name::<Self>());
std::sync::Arc::downgrade(&std::sync::Arc::new(T::example()))
info!("AbiExample for (Arc's Weak<T>): {}", type_name::<Self>());
// leaking is needed otherwise Arc::upgrade() will always return None...
std::sync::Arc::downgrade(leak_and_inhibit_drop(std::sync::Arc::new(T::example())))
}
}

Expand All @@ -330,6 +340,14 @@ impl<T: AbiExample> AbiExample for std::rc::Rc<T> {
}
}

impl<T: AbiExample> AbiExample for std::rc::Weak<T> {
fn example() -> Self {
info!("AbiExample for (Rc's Weak<T>): {}", type_name::<Self>());
// leaking is needed otherwise Rc::upgrade() will always return None...
std::rc::Rc::downgrade(leak_and_inhibit_drop(std::rc::Rc::new(T::example())))
}
}

impl<T: AbiExample> AbiExample for std::sync::Mutex<T> {
fn example() -> Self {
info!("AbiExample for (Mutex<T>): {}", type_name::<Self>());
Expand Down