crossbeam-rs/crossbeam-epoch

Guard::defer is a footgun

Opened this issue · 5 comments

Deleted user commented

@Amanieu just hit an annoying bug where he forgot to write move before the closure:

guard.defer(|| Self::finalize(self));

The documentation of defer suggests always using move, but it's too easy to forget about this rule and run into very difficult to find bugs. Clearly, we should do something here.

My suggestion is to have two variants of defer, a checked and an unchecked version (similar to get vs get_unchecked, or unreachable!() vs unreachable_unchecked()):

fn defer<F, R>(&self, f: F)
where
    F: FnOnce() -> R + Send + 'static;

unsafe fn defer_unchecked<F, R>(&self, f: F)
where
    F: FnOnce() -> R;

That means the original call to defer would be written as:

let ptr = self as *const _ as usize;
guard.defer(|| Self::finalize(ptr as *const Self));

Or, using the more dangerous option:

guard.defer_unchecked(move || Self::finalize(self));

This situation reminds me of spawn_unsafe (it should really be named spawn_unchecked), which is equivalent to std::thread::spawn, except it doesn't require F: 'static (and the F: Send bound should probably be lifted too, IMO; see this).

jeehoonkang commented

I'm just asking -- Did self implement Copy?

I agree with @stjepang on the naming and the signature of spawn_unsafe.

Amanieu commented

In my example, finalize takes *const T and self is &self.

Deleted user commented

Ok, so two questions:

First, what do y'all think about having defer and defer_unchecked? Would that reduce the danger of the footgun? I like this distinction because defer's interface would precisely specify the requirements (F: Send + 'static), while defer_unchecked should give everyone a pause because it sounds more scary.

And second question: How about adding the following method to Guard? Do you think it'd be helpful, too? After all, this is the most common use case for deferred destruction and it'd be hard to misuse:

unsafe fn defer_destroy<T>(&self, ptr: Shared<T>) {
    self.defer_unchecked(move || ptr.into_owned());
}
Amanieu commented

Having both defer and defer_unchecked sounds good! And I also like defer_destroy (bikeshed: defer_free, defer_drop).