[Merged by Bors] - Fix Events::<drain/clear> bug

[NathanSWard]

Taken from #2145

On draining and clearing, dangling EventReaders would not read into the correct event offset.

[alice-i-cecile]

Thanks! This is a nice thing to pull over.

[cart]

Good call. Looks like events.clear() also needs to reset its counters. I'll put together a follow up.

[cart]

bors r+

[cart]

bors r-

[bors]

Canceled.

[cart]

Hmm one issue to discuss that I discovered when putting together my followup:

Resetting event_count means that existing event readers will skip over newly sent events? The test looks like it should fail, so I need to understand why it works before merging :)

[cart]

[NathanSWard]

Good call. Looks like events.clear() also needs to reset its counters. I'll put together a follow up.

@cart I'll add this fix to the PR as well.

Resetting event_count means that existing event readers will skip over newly sent events? The test looks like it should fail, so I need to understand why it works before merging :)

Hmm yes this does seem to be incorrect...
Looks like we might need to add a dirty_flag or something of the sort.

[cart]

This makes the test above pass, but it feels like I'm missing a corner case:

[NathanSWard]

Yep adding this:

        events.send(E(1));
        let _ = events.drain();
        assert_eq!(reader.iter(&events).next(), None);

in the tests causes it to fail.

[cart]

Yep adding this:

Is that with or without the "fix" above? Adding that passes with the fix for me.

[NathanSWard]

@cart, a possible solution:
we might need to add a version field to Events and Event<Reader/Writer> and on calling clear/drain, update the version. So when the Reader/Writer calls .iter() check the versions and reset the Reader/Writer's internal count to 0 if the version has changed.

[NathanSWard]

Is that with or without the "fix" above? Adding that passes with the fix for me.

With your change included.
here is the full test i have:

#[test]
    fn test_events_drain() {
        #[derive(PartialEq, Eq, Debug)]
        struct E(usize);

        let mut events = Events::<E>::default();
        let mut reader = events.get_reader();

        assert!(reader.iter(&events).next().is_none());

        events.send(E(0));
        assert_eq!(reader.iter(&events).next(), Some(&E(0)));
        assert_eq!(reader.iter(&events).next(), None);

        events.send(E(1));
        let _ = events.drain();
        assert_eq!(reader.iter(&events).next(), None);

        events.send(E(2));
        events.send(E(3));
        assert_eq!(reader.iter(&events).next(), Some(&E(2)));
        assert_eq!(reader.iter(&events).next(), Some(&E(3)));
        assert_eq!(reader.iter(&events).next(), None);
    }

[cart]

Found another corner case. Adding an events.update() breaks it.

#[test]
fn test_events_drain() {
    #[derive(PartialEq, Eq, Debug)]
    struct E(usize);
    let mut events = Events::<E>::default();
    let mut reader = events.get_reader();

    assert!(reader.iter(&events).next().is_none());

    events.send(E(0));
    assert_eq!(*reader.iter(&events).next().unwrap(), E(0));
    assert_eq!(reader.iter(&events).next(), None);

    let _ = events.drain();
    assert!(reader.iter(&events).next().is_none());

    events.send(E(1));
    events.update();
    events.send(E(2));
    assert_eq!(*reader.iter(&events).next().unwrap(), E(1));
    assert_eq!(*reader.iter(&events).next().unwrap(), E(2));
}

[cart]

I'm gonna keep working through my PR review queue. If you find a fix definitely do it for clear() too 😄

I'll check back in later / help out if it hasn't been sorted out yet.

[NathanSWard]

@cart it turns out our last #[test]s are actually invalid... haha 😅

reader.iter(&events) returns an iterator over all events.
We were calling it multiple times expecting it to return a single value.

This is how the test should be structured...

#[test]
    fn test_events_drain() {
        // test stuff

        events.send(E(1));
        events.update();
        events.send(E(2));

        let mut iter = reader.iter(&events);
        assert_eq!(*iter.next().unwrap(), E(1));
        assert_eq!(*iter.next().unwrap(), E(2));
    }

[cart]

Bahaha whoops. Nice catch!

[alice-i-cecile]

Good; factoring out that logic reduces the risk of bugs in the future.

[NathanSWard]

@cart I have this current implementation which passed the failing test we set up.
I'm currently toying around with other potential failing tests, and please let me know if you can think of any :)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-doc
• check-markdown-links
• markdownlint
• run-examples