Change to WakeCounter API proposal

[Thomasdezeeuw]

I would like to propose a breaking change in API for the WakerCounter type in the futures-test crate. The current API has the counter and LocalWaker implementation in the same WakerCounter type, which can be limiting. I propose an API that splits it into two types. Since it's not a lot of code I have a working implementation below.

/// Create a new `LocalWaker` that counts the number of times it's awoken.
pub fn new_count_waker() -> (LocalWaker, AwokenCount) {
    let inner = Arc::new(WakerInner { count: AtomicUsize::new(0) });
    (local_waker_from_nonlocal(inner.clone()), AwokenCount { inner })
}

/// Number of times the waker was awoken.
///
/// See [`new_count_waker`].
pub struct AwokenCount {
    inner: Arc<WakerInner>,
}

impl AwokenCount {
    /// Get the number of times the waker was awoken.
    pub fn get(&self) -> usize {
        self.inner.count.load(Ordering::SeqCst)
    }
}

struct WakerInner {
    count: AtomicUsize,
}

impl Wake for WakerInner {
    fn wake(arc_self: &Arc<Self>) {
        let _ = arc_self.count.fetch_add(1, Ordering::SeqCst);
    }
}

And the usage.

let (waker, count) = new_count_waker();

// Polling is easy since we just have a `LocalWaker`.
let future = // ...
future.poll(&waker);

// And elsewhere we can check the wake count.
assert_eq!(count.get(), 1);

[Matthias247]

I think that's a reasonable API. However since this is mostly a test API, I think the current one does it's job pretty well too, and there's not too benefit from splitting things into a nicer API (with separation of concerns). Maybe we can just add a derive for Clone on the current implementation for the use-cases that are currently not covered?

If things are changed, maybe AwokenCount could also implement Deref or Into for usize. But since comparisons afaik don't automatically use those, and comparing with *&count looks less obvious than count.get(), it's probably also not worth it.

[Thomasdezeeuw]

I think that's a reasonable API.

Is reasonable better then the current API, or worse?

However since this is mostly a test API, I think the current one does it's job pretty well too, and there's not too benefit from splitting things into a nicer API (with separation of concerns).

I have to say that I strongly disagree, even for a test API it is still code we have to live with and use on a daily basis. Eventhough it's test code doesn't mean it should meet quality standards.

Maybe we can just add a derive for Clone on the current implementation for the use-cases that are currently not covered?

That would be a good start.

If things are changed, maybe AwokenCount could also implement Deref or Into for usize. But since comparisons afaik don't automatically use those, and comparing with *&count looks less obvious than count.get(), it's probably also not worth it.

[Thomasdezeeuw]

Any other opinions? Should a create a PR should people can try the new API?

[Nemo157]

If things are changed, maybe AwokenCount could also implement Deref or Into for usize. But since comparisons afaik don't automatically use those, and comparing with *&count looks less obvious than count.get(), it's probably also not worth it.

Alternatively it could have impl PartialEq<usize> for AwokenCount which would allow assert_eq!(count, 5) (but not the other order).

---

I agree with your position that test code should be of good quality (and almost believe that readability of tests matters more than readability of the implementation) so if we can provide a better API here we should.

I wonder if there's a way to achieve something similar without having to provide a tuple return value. What if WakeCounter implemented Clone + PartialEq<usize> + Deref<Item = LocalWaker> (I think that's possible, but haven't checked). That might give something like:

let counter = new_count_waker();

// Polling is easy since we deref to `LocalWaker`.
let future = // ...
future.poll(&counter);

// If we need to send owned copies that's also easy
{
    let counter = counter.clone();
    thread::spawn(move || foo().poll(&counter);
}
// or maybe just emphasize the current clonability of the waker in the docs
{
    let lw = counter.local_waker().clone();
    thread::spawn(move || foo().poll(&lw));
}

// And we can check the wake count.
assert_eq!(counter, 1);

[Thomasdezeeuw]

Alternatively it could have impl PartialEq<usize> for AwokenCount which would allow assert_eq!(count, 5) (but not the other order).

I really like the idea of implementing PartialEq, I didn't even think about that. By default {integer} types (i.e. integer without a type) are i32 I think, so we might have to implement the trait for some more types other then usize.

I wonder if there's a way to achieve something similar without having to provide a tuple return value. What if WakeCounter implemented Clone + PartialEq<usize> + Deref<Item = LocalWaker> (I think that's possible, but haven't checked). That might give something like:

Have something against return a tuple? (I'm just curious) I personally like the split between the two types making them both simple to use.

[Nemo157]

Inference to usize from the assert_eq seemed to work fine in my very brief testing of the idea (implementing the trait for more types would probably break this, it only works because there's only a single type it can be equated to).

I don't really have much against tuple returns other than having to think up an extra name for each variable (although in this case I guess it's pretty self-evident). It probably makes more sense than abusing Deref in this case as well.

[Thomasdezeeuw]

@Nemo157 Should I create a pr with the API?

[Nemo157]

Sounds good to me