Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable passing external guards into Rune #269

Merged
merged 2 commits into from Jun 15, 2021

Conversation

tgolsson
Copy link
Contributor

The specific use-case for me is to box up a Mutex guard and drop it when the inner goes out of scope, but this is essentially just an "on-dropped" callback with user-data... Maybe the bar should be a bit higher to prevent abuse. I think with some cleanup this'll be something like Shared::from_{ref_mut}_with_guard, but for now I've made a bunch of things public. Having so many core types private makes it really hard to hack at this kind of API (because it needs to be in the slow-to-compile Rune instead of a client crate.

@tgolsson
Copy link
Contributor Author

This is an example of how I'm doing it in the client crate at the moment:

impl Transform {
    fn get_mut(world: &WorldProxy, entity: &EntityProxy) -> Result<Option<Value>, VmError> {
        let comps = world.world.get_mut::<Components<Transform>>().expect("TODO[TSolberg]");

        let comp = AtomicRefMut::filter_map(comps, |comps| comps.get_mut(entity.entity));
        let comp = match comp {
            Some(e) => e,
            None => {
                return Ok(None);
            }
        };

        let (value, guard) = unsafe { comp.into_raw() };

        let ptr = Box::into_raw(Box::new(guard));

        let inner = NonNull::from(Box::leak(Box::new(SharedBox {
            access: Access::new(true),
            count: Cell::new(1),
            data: unsafe { AnyObj::from_mut(value).into() },
            guard: Some(RawDrop {
                data: ptr as *const (),
                drop_fn: |user_data| unsafe {
                    drop(Box::from_raw(user_data as *mut AtomicBorrowRefMut));
                },
            }),
        })));

        let shared = Shared { inner };
        Ok(Some(shared.into()))
    }
}

@udoprog
Copy link
Collaborator

udoprog commented Jun 12, 2021

Neat. All though I'm not entirely happy about making so many raw edges public!

My immediate thought is that this would probably fit better in AnyObj, which already uses a customizable vtable that I think is flexible enough to implement what you need. Something with a public API like this:

let any_obj = AnyObj::from_pointer_and_guard(value, guard);

What it needs is that the data pointer points to a combination of the pointer and the owned guard:

struct PointerAndGuard {
    value: *const (),
    guard: *const (),
}

And an as_ptr implementation which checks for the type of Value, and then deferences the PointerAndGuard struct:

/// `T` here would be the type of the value being pointed to.
fn pointer_and_guard_as_ptr_impl<T>(this: *const (), expected: Hash) -> Option<*const ()>
where
    T: Any,
{
    if expected == Hash::from_type_id(any::TypeId::of::<T>()) {
        let this = this as *const PointerAndGuard<T>;
        Some((*this).value)
    } else {
        None
    }
}

Finally a drop implementation which just drops the guard. But I'll leave that as an exercise to the reader ;)

@udoprog udoprog added the enhancement New feature or request label Jun 12, 2021
@tgolsson
Copy link
Contributor Author

Thanks for the feedback @udoprog! I revised and simplifed a lot. Much simpler and less dangerous. I'll take another pass on documentation tomorrow and then this should be G2G.

Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Really neat to piggyback on Deref / DerefMut. Gives the function a really broad set of utility.

I need to take an even harder think about any potential bad things that can happen due to leaking a lifetimed guard like this. But since the function is unsafe and is primarily intended to be used in limited scope-like manners we're probably good to go for now.

A few style and naming nits. Other than that, merge when you feel like it. Awesome work!

crates/runestick/src/any_obj.rs Outdated Show resolved Hide resolved
crates/runestick/src/any_obj.rs Outdated Show resolved Hide resolved
crates/runestick/src/any_obj.rs Outdated Show resolved Hide resolved
crates/runestick/src/any_obj.rs Outdated Show resolved Hide resolved
@tgolsson tgolsson marked this pull request as ready for review June 14, 2021 20:05
@udoprog
Copy link
Collaborator

udoprog commented Jun 15, 2021

Pushed one more change which removed U as a type parameters. I realized it's not needed since Target is an associated type. If you're cool with it, feel free to merge. 👍

@tgolsson tgolsson merged commit 1a3b1b1 into rune-rs:main Jun 15, 2021
@tgolsson tgolsson deleted the ts/external-guards branch June 15, 2021 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants