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

core/events: refactor the events.Target to use mutexes instead of a background goroutine #4700

Merged
merged 3 commits into from Nov 2, 2023

Conversation

calebdoxsey
Copy link
Contributor

@calebdoxsey calebdoxsey commented Nov 1, 2023

Summary

Currently the event target pushes an operation onto a channel for each of its methods. This is then handled by a goroutine. Since each of the operations goes through a separate channel, and the channels were defined with a size of 1, it was possible for call order to behave unpredictably. For example:
AddListener(li)
Dispatch(ctx, evt)

Could end up with the Dispatch occurring before AddListener.

By making these channels synchronous we can avoid this behavior. Now AddListener will block until the goroutine picks it up, and Dispatch will always happen after AddListener.

Refactor the events.Target to use mutexes instead of a background goroutine.

@calebdoxsey calebdoxsey requested a review from a team as a code owner November 1, 2023 21:33
@coveralls
Copy link

coveralls commented Nov 1, 2023

Coverage Status

coverage: 64.134% (+0.02%) from 64.112% when pulling 0a769f7 on cdoxsey/events-ordering into 0b79a28 on main.

@kenjenkins
Copy link
Contributor

kenjenkins commented Nov 2, 2023

I'm not sure this is sufficient to order the execution of the AddListener() / Dispatch() operations.

Consider the following test case:

func TestDispatchOrder(t *testing.T) {
    var target events.Target[int64]
    t.Cleanup(target.Close)

    ch := make(chan error)
    
    var next atomic.Int64
    
    update := func(_ context.Context, i int64) {
        if n := next.Load(); i != n {
            ch <- fmt.Errorf("want %d, got %d", n, i)
        }
        next.Store(i + 1)
    }
    
    go func() {
        for i := int64(0); i < 1000; i++ {
            h := target.AddListener(update)
            target.Dispatch(context.Background(), i)
            target.RemoveListener(h)
        }   
    }()
    
    if err := <-ch; err != nil {
        t.Fatal(err)
    }   
}

Should it pass after this PR?

It seems to fail reliably on my machine at some point before all 1000 iterations.

(Let me know if I've made some mistake in the test case itself.)

[edit: indeed, this test is buggy in a few ways — most importantly, even if the dispatches all occur in order, the different listener goroutines will be racing to process the events, so there's no guarantee the update function will be called with each event in the order they were dispatched]

@calebdoxsey
Copy link
Contributor Author

I will refactor this to use mutexes.

@calebdoxsey calebdoxsey marked this pull request as draft November 2, 2023 14:01
@calebdoxsey calebdoxsey changed the title core/events: use synchronous channel to fix flaky test core/events: fix flaky test Nov 2, 2023
@calebdoxsey calebdoxsey marked this pull request as ready for review November 2, 2023 14:38
@calebdoxsey calebdoxsey merged commit 5f4e13e into main Nov 2, 2023
9 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/events-ordering branch November 2, 2023 17:28
@wasaga wasaga added bug Something isn't working and removed bug Something isn't working labels Nov 16, 2023
@wasaga wasaga changed the title core/events: fix flaky test core/events: refactor the events.Target to use mutexes instead of a background goroutine Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants