-
Notifications
You must be signed in to change notification settings - Fork 50
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
refactor: Use events to test network logic #2700
Changes from 18 commits
b3f45bc
10c522b
8f870bd
138bb84
144b392
243775a
6b8d97e
05f1579
98e77d4
2da9153
3725fca
45f6edd
938954e
6e98699
1a1ce8f
9ce688f
7e1045f
573090e
ab56124
d6a1be1
23d4d95
64ebf82
42c52d0
7ac87b9
311a37d
cfa0ef9
0357385
3513390
184f0cc
38e0c48
55aac76
e6128ef
801354c
2dfa4d2
56136c5
726be78
61ab97f
9fb2eeb
72f44ce
d0ad720
aeec15d
7680133
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package event | ||
|
||
import ( | ||
"sync/atomic" | ||
) | ||
|
||
type subscribeCommand *Subscription | ||
|
||
type unsubscribeCommand *Subscription | ||
|
||
type publishCommand Message | ||
|
||
type closeCommand struct{} | ||
|
||
// bufferedBus is a bus that uses a buffered channel to manage subscribers and publish messages. | ||
type bufferedBus struct { | ||
// subID is incremented for each subscriber and used to set subscriber ids. | ||
subID atomic.Uint64 | ||
// subs is a mapping of subscriber ids to subscriptions. | ||
subs map[uint64]*Subscription | ||
// events is a mapping of event names to subscriber ids. | ||
events map[string]map[uint64]struct{} | ||
// commandChannel manages all commands sent to this simpleChannel. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: what are you referring to by "simpleChannel"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. It was a copy paste error. |
||
// | ||
// It is important that all stuff gets sent through this single channel to ensure | ||
// that the order of operations is preserved. | ||
// | ||
// WARNING: This does mean that non-event commands can block the database if the buffer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: sounds more like a NOTE, not a WARNING |
||
// size is breached (e.g. if many subscribe commands occupy the buffer). | ||
commandChannel chan any | ||
eventBufferSize int | ||
hasClosedChan chan struct{} | ||
isClosed atomic.Bool | ||
} | ||
|
||
// NewBufferedBus creates a new event bus with the given commandBufferSize and | ||
// eventBufferSize. | ||
// | ||
// Should the buffers be filled, subsequent calls on this bus will block. | ||
func NewBufferedBus(commandBufferSize int, eventBufferSize int) Bus { | ||
bus := bufferedBus{ | ||
subs: make(map[uint64]*Subscription), | ||
events: make(map[string]map[uint64]struct{}), | ||
commandChannel: make(chan any, commandBufferSize), | ||
hasClosedChan: make(chan struct{}), | ||
eventBufferSize: eventBufferSize, | ||
} | ||
go bus.handleChannel() | ||
return &bus | ||
} | ||
|
||
func (b *bufferedBus) Publish(msg Message) { | ||
if b.isClosed.Load() { | ||
return | ||
} | ||
b.commandChannel <- publishCommand(msg) | ||
} | ||
|
||
func (b *bufferedBus) Subscribe(events ...string) (*Subscription, error) { | ||
if b.isClosed.Load() { | ||
return nil, ErrSubscribedToClosedChan | ||
} | ||
sub := &Subscription{ | ||
id: b.subID.Add(1), | ||
value: make(chan Message, b.eventBufferSize), | ||
events: events, | ||
} | ||
b.commandChannel <- subscribeCommand(sub) | ||
return sub, nil | ||
} | ||
|
||
func (b *bufferedBus) Unsubscribe(sub *Subscription) { | ||
if b.isClosed.Load() { | ||
return | ||
} | ||
b.commandChannel <- unsubscribeCommand(sub) | ||
} | ||
|
||
func (b *bufferedBus) Close() { | ||
if b.isClosed.Load() { | ||
return | ||
} | ||
b.isClosed.Store(true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: these 2 ops are not done atomically and might close twice. Please use compare-and-swap: if !b.isClosed.CompareAndSwap(false, true) {
return
}
... Check please if you have similar occurrences. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch! |
||
b.commandChannel <- closeCommand{} | ||
// Wait for the close command to be handled, in order, before returning | ||
<-b.hasClosedChan | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: this is thread-safe only under the assumption that all the calls to Otherwise the following scenario is possible:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've replaced the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's quite a broad mutex, please don't merge yet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should only block when calling close. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think I understand, and there is no nicer way of handling this that I can spot off the top of my head. todo: Can you please rename todo: Can please also document the lock on its declaration in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AndrewSisley can you explain what you mean by broad? We use the same technique in the memory store and I think it's really fine for this as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The lock spans every public call to this struct, it can't really get any broader :) If it only locks whilst closing that's fine, but it would be very damaging if that were to change (hence the documentation and name change requested). The current name ( |
||
} | ||
|
||
func (b *bufferedBus) handleChannel() { | ||
for cmd := range b.commandChannel { | ||
switch t := cmd.(type) { | ||
case closeCommand: | ||
for _, subscriber := range b.subs { | ||
close(subscriber.value) | ||
} | ||
close(b.commandChannel) | ||
close(b.hasClosedChan) | ||
return | ||
|
||
case subscribeCommand: | ||
for _, event := range t.events { | ||
if _, ok := b.events[event]; !ok { | ||
b.events[event] = make(map[uint64]struct{}) | ||
} | ||
b.events[event][t.id] = struct{}{} | ||
} | ||
b.subs[t.id] = t | ||
|
||
case unsubscribeCommand: | ||
if _, ok := b.subs[t.id]; !ok { | ||
continue // not subscribed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Just pointing incase easy to test, that this path is not tested. |
||
} | ||
for _, event := range t.events { | ||
delete(b.events[event], t.id) | ||
} | ||
delete(b.subs, t.id) | ||
close(t.value) | ||
|
||
case publishCommand: | ||
for id := range b.events[WildCardEventName] { | ||
b.subs[id].value <- Message(t) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Just pointing incase easy to test, that this path is not tested. |
||
} | ||
for id := range b.events[t.Name] { | ||
if _, ok := b.events[WildCardEventName][id]; ok { | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Just pointing incase easy to test, that this path is not tested. |
||
} | ||
b.subs[id].value <- Message(t) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package event | ||
|
||
import ( | ||
"sync" | ||
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestSimplePushIsNotBlockedWithoutSubscribers(t *testing.T) { | ||
bus := NewBufferedBus(0, 0) | ||
defer bus.Close() | ||
|
||
msg := NewMessage("test", 1) | ||
bus.Publish(msg) | ||
|
||
// just assert that we reach this line, for the sake of having an assert | ||
assert.True(t, true) | ||
} | ||
|
||
func TestSimpleSubscribersAreNotBlockedAfterClose(t *testing.T) { | ||
bus := NewBufferedBus(0, 0) | ||
defer bus.Close() | ||
|
||
sub, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: it's better to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was copy pasted, but I've fixed them all. |
||
|
||
bus.Close() | ||
|
||
<-sub.Message() | ||
|
||
// just assert that we reach this line, for the sake of having an assert | ||
assert.True(t, true) | ||
} | ||
|
||
func TestSimpleEachSubscribersRecievesEachItem(t *testing.T) { | ||
bus := NewBufferedBus(0, 0) | ||
defer bus.Close() | ||
|
||
msg1 := NewMessage("test", 1) | ||
msg2 := NewMessage("test", 2) | ||
|
||
sub1, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
|
||
sub2, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
|
||
// ordering of publish is not deterministic | ||
// so capture each in a go routine | ||
var wg sync.WaitGroup | ||
var event1 Message | ||
var event2 Message | ||
|
||
go func() { | ||
event1 = <-sub1.Message() | ||
wg.Done() | ||
}() | ||
|
||
go func() { | ||
event2 = <-sub2.Message() | ||
wg.Done() | ||
}() | ||
|
||
wg.Add(2) | ||
bus.Publish(msg1) | ||
wg.Wait() | ||
|
||
assert.Equal(t, msg1, event1) | ||
assert.Equal(t, msg1, event2) | ||
|
||
go func() { | ||
event1 = <-sub1.Message() | ||
wg.Done() | ||
}() | ||
|
||
go func() { | ||
event2 = <-sub2.Message() | ||
wg.Done() | ||
}() | ||
|
||
wg.Add(2) | ||
bus.Publish(msg2) | ||
wg.Wait() | ||
|
||
assert.Equal(t, msg2, event1) | ||
assert.Equal(t, msg2, event2) | ||
} | ||
|
||
func TestSimpleEachSubscribersRecievesEachItemGivenBufferedEventChan(t *testing.T) { | ||
bus := NewBufferedBus(0, 2) | ||
defer bus.Close() | ||
|
||
msg1 := NewMessage("test", 1) | ||
msg2 := NewMessage("test", 2) | ||
|
||
sub1, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
sub2, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
|
||
// both inputs are added first before read, using the internal chan buffer | ||
bus.Publish(msg1) | ||
bus.Publish(msg2) | ||
|
||
output1Ch1 := <-sub1.Message() | ||
output1Ch2 := <-sub2.Message() | ||
|
||
output2Ch1 := <-sub1.Message() | ||
output2Ch2 := <-sub2.Message() | ||
|
||
assert.Equal(t, msg1, output1Ch1) | ||
assert.Equal(t, msg1, output1Ch2) | ||
|
||
assert.Equal(t, msg2, output2Ch1) | ||
assert.Equal(t, msg2, output2Ch2) | ||
} | ||
|
||
func TestSimpleSubscribersDontRecieveItemsAfterUnsubscribing(t *testing.T) { | ||
bus := NewBufferedBus(0, 0) | ||
defer bus.Close() | ||
|
||
sub, err := bus.Subscribe("test") | ||
assert.Nil(t, err) | ||
bus.Unsubscribe(sub) | ||
|
||
msg := NewMessage("test", 1) | ||
bus.Publish(msg) | ||
|
||
// tiny delay to try and make sure the internal logic would have had time | ||
// to do its thing with the pushed item. | ||
time.Sleep(5 * time.Millisecond) | ||
|
||
// closing the channel will result in reads yielding the default value | ||
assert.Equal(t, Message{}, <-sub.Message()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Please add a compile time check to ensure (and document) that this type implements the
Bus
interface.i.e.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor method does this already. I usually choose one or the other not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough :) Fred might ask you to change the constructor to return the concrete though 😁