refactor: Use events to test network logic #2700

nasdf · 2024-06-07T20:25:53Z

Relevant issue(s)

Resolves #2699
Resolves #2687

Description

This PR refactors the events package and updates the networking tests to use events.

I think there is a bit more clean up to be done, but the majority should be ready for review.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

tests/integration/events/utils.go

events/bus.go

fredcarle

I need to head out but first pass looks really good. I love that you removed 1000 lines of code 🤘. I'll finish a more in depth review on Monday.

codecov · 2024-06-07T20:57:35Z

Codecov Report

Attention: Patch coverage is 90.62500% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.11%. Comparing base (c7fc43d) to head (7680133).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2700      +/-   ##
===========================================
- Coverage    78.13%   78.11%   -0.02%     
===========================================
  Files          311      310       -1     
  Lines        23238    23069     -169     
===========================================
- Hits         18155    18019     -136     
+ Misses        3699     3675      -24     
+ Partials      1384     1375       -9

Flag	Coverage Δ
all-tests	`78.11% <90.62%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
cli/start.go	`45.61% <ø> (+0.79%)`	⬆️
event/event.go	`100.00% <100.00%> (ø)`
internal/db/collection.go	`71.60% <100.00%> (-0.23%)`	⬇️
internal/db/collection_delete.go	`39.53% <100.00%> (-2.69%)`	⬇️
internal/db/config.go	`100.00% <ø> (ø)`
net/client.go	`100.00% <100.00%> (ø)`
net/sync_dag.go	`82.35% <100.00%> (+0.53%)`	⬆️
http/client.go	`55.79% <0.00%> (ø)`
event/bus.go	`97.10% <97.10%> (ø)`
internal/db/db.go	`60.71% <71.43%> (-2.76%)`	⬇️
... and 5 more

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7fc43d...7680133. Read the comment docs.

tests/integration/p2p.go

tests/integration/utils2.go

tests/integration/p2p.go

AndrewSisley

I think we will need to chat about this one - it might be worth separating the changes made outside of the events package from the rest - there are some very good changes in here that should not be delayed/confused with the events package implementation change.

Regarding the events package, it is now simpler, but it is also blocking (in user-space), and less reliable (it discards messages quite rapidly if the subscription buffer fills - the original buffer size was 100 + a 60 second timeout, it is now 10 items and no timeout) and as such I think I will require a bit of convincing in order to see this as a good change.

I do also disagree with the removal of the Channel interface, it was used to allow us to swap out implementations without having to change the calling code. Whilst I think the original simple channel is better than the proposal for our use-cases, it still needs to be improved upon long-term, and different use-cases within the defra code base would likely benefit from different Channel implementations.

tests/integration/state.go

events/bus.go

fredcarle · 2024-06-10T18:12:21Z

events/events.go

-	Unsubscribe(Subscription[T])
+// ConnectEvent is an event that is published when
+// a peer connection has changed status.
+type ConnectEvent = event.EvtPeerConnectednessChanged


suggestion: Since we are in the events package, the Event suffix isn't necessary. We could change events.ConnectEvent, events.PubSubEvent, events.UpdateEvent, events.MergeEvent to events.Connect, events.PubSub, events.Update, events.Merge

that would be neat. But for this, following best go practices, would be better to make package name singular event.

@nasdf you changed to event but didn't do my suggestion. Did you just forget or did you decide you didn't like it? :)

@fredcarle I'm considering moving the event definitions to their respective packages.

Also a good option 👍

The downside of defining the events in their respective packages is that if might result in unwanted dependencies or even circular dependencies depending in which package it is defined. MergeEvent being defined in db or net for example.

Good point. I've kept them in the package but renamed as you suggested.

this is being discussed in another thread #2700 (comment) :)

events/bus.go

events/bus_test.go

islamaliev · 2024-06-10T19:59:29Z

events/events.go

+const (
+	// WildCardEventName is the alias used to subscribe to all events.
+	WildCardEventName = "*"
+	// MergeCompleteEventName is the name of the database merge complete event.
+	MergeCompleteEventName = "db:merge-complete"
+	// UpdateEventName is the name of the database update event.
+	UpdateEventName = "db:update"
+	// ResultsEventName is the name of the database results event.
+	ResultsEventName = "db:results"
+	// MergeRequestEventName is the name of the net merge request event.
+	MergeRequestEventName = "net:merge"
+	// PubSubEventName is the name of the network pubsub event.
+	PubSubEventName = "net:pubsub"
+	// ConnectEventName is the name of the network connect event.
+	ConnectEventName = "net:connect"
+)


suggestion: wouldn't it be better to make name an enum (for more explicitness) and make every package (in this case db and net) own the events they generate.
This way this file won't need to change every time a new event in an unrelated package is added.

I'd like to keep the event names a human readable string for debugging purposes.

Having the events in one package does have the issue you mentioned, but it also makes it much easier to know what events can be subscribed to. There's also the issue of importing an entire package just for an event type.

but the package that wants to listed to a specific event usually depends on the package that fires that event as it needs to receive the package-event-specific object.

I'd like to keep the event names a human readable string for debugging purposes

You can declare string enum:

type EventName string

and in db package:

const ( // MergeCompleteEventName is the name of the database merge complete event. MergeCompleteEventName = events.EventName("db:merge-complete") // UpdateEventName is the name of the database update event. UpdateEventName = events.EventName("db:update") // ResultsEventName is the name of the database results event. ResultsEventName = events.EventName("db:results") )

but the package that wants to listed to a specific event usually depends on the package that fires that event as it needs to receive the package-event-specific object.

This won't always be the case, and will cause problems since the db package is internal.

You can declare string enum:

I've added an event.Name type.

tests/integration/events/utils.go

islamaliev · 2024-06-13T21:52:15Z

event/buffered_bus.go

+	}
+	b.commandChannel <- closeCommand{}
+	// Wait for the close command to be handled, in order, before returning
+	<-b.hasClosedChan


question: this is thread-safe only under the assumption that all the calls to bufferedBus are done by the same thread. I'm not sure this is the case. Is it?

Otherwise the following scenario is possible:

execution enters Subscribe method, b.isClosed == false so it proceeds but pauses just before b.commandChannel <- subscribeCommand(sub)

concurrently Close is being called, sets b.isClosed to true, pushes b.commandChannel <- closeCommand{}

handleChannel receives closeCommand and executes close(b.commandChannel)

now the thread from step 1 resumes, executes b.commandChannel <- subscribeCommand(sub) and causes a panic.

I've replaced the isClosed atomic with a RWMutex. Issue should be fixed now.

That's quite a broad mutex, please don't merge yet

It should only block when calling close.

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 mutex to closeLock or similar, we really really don't want this to be used for anything else.

todo: Can please also document the lock on its declaration in bufferedBus, explaining that it is only locked whilst closing.

@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.

@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.

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 (mutex) is just begging for other places to lock it besides Close in the future.

shahzadlone · 2024-06-17T14:46:31Z

event/buffered_bus.go

+
+		case unsubscribeCommand:
+			if _, ok := b.subs[t.id]; !ok {
+				continue // not subscribed


question: Just pointing incase easy to test, that this path is not tested.

shahzadlone · 2024-06-17T14:46:58Z

event/buffered_bus.go

+
+		case publishCommand:
+			for id := range b.events[WildCardName] {
+				b.subs[id].value <- Message(t)


question: Just pointing incase easy to test, that this path is not tested.

shahzadlone · 2024-06-17T14:47:20Z

event/buffered_bus.go

+			}
+			for id := range b.events[t.Name] {
+				if _, ok := b.events[WildCardName][id]; ok {
+					continue


question: Just pointing incase easy to test, that this path is not tested.

shahzadlone · 2024-06-17T14:48:14Z

event/bus.go

+
+// Events returns the names of all subscribed events.
+func (s *Subscription) Events() []Name {
+	return s.events


nitpick: This path is not tested.

islamaliev · 2024-06-17T20:40:29Z

event/buffered_bus_test.go

+	defer bus.Close()
+
+	sub, err := bus.Subscribe("test")
+	assert.Nil(t, err)


suggestion: it's better to use assert.NoError because it produces better output

It was copy pasted, but I've fixed them all.

islamaliev · 2024-06-17T20:46:08Z

event/buffered_bus_test.go

+	assert.True(t, true)
+}
+
+func TestBufferedBusUnsubscribeTwice(t *testing.T) {


suggestion: some time ago we discussed a naming convention we want to use for our tests and agreed to name them using the following patter:
<test_group>_<condition>_<expectation>.
For this particular test it might be something like TestBufferedBud_IfUnsubscribedTwice_[Succeed|NoError|NoPanic] which makes it much more explicit.
For the test above TestBufferedBus_IfClosed_SubscribersAreNotBlocked

I've updated all the test names

this is not quite what I meant.
The part is what makes this test different from others. It describes first 2 As (arrange and action).
The part describes the 3rd A: assert.
For example your new test name is TestBus_WildCardDeduplicates_Succeed. In this case part might be IfSubscriptionsOverlap or IfSubscribedToEventAndWildCard or IfSubscribedToMultipleEvents. The part could be ItShouldDeduplicate or ShouldReceiveOnlyOneMessage. So together it would be something like TestBus_IfSubscribedToEventAndWildCard_ItShouldDeduplicate.

I'm not sure if I explained it clearly. So I will give some more examples how the current names can be transformed:
TestBus_EachSubscribersRecievesEachItem_Succeed -> TestBus_WithMultipleSubscriptions_EachShouldReceiveMessage or TestBus_UponPublishWhileMultipleSubscriptions_EachShouldReceiveMessage

TestBus_SubscribersDontRecieveItemsAfterUnsubscribing_Succeed -> TestBus_AfterUnsubscribing_SubscribersDontRecieveItems or TestBus_IfUnsubscribed_ShouldNotReceivePublishedMessages

Sorry for a long lecture. Just wanted to make sure it's clear.

islamaliev · 2024-06-17T20:52:03Z

event/buffered_bus_test.go

+	assert.NoError(t, err)
+
+	bus.Unsubscribe(sub)
+	bus.Unsubscribe(sub)


suggestion: if it is no panicking that you are asserting, why no to use assert.NotPanics?

This call itself won't panic because it is simply adding a message to the command channel which is processed asynchronously.

islamaliev · 2024-06-17T20:55:14Z

event/buffered_bus_test.go

+	select {
+	case <-sub.Message():
+		t.Errorf("should not receive duplicate message")
+	case <-time.After(1 * time.Second):


todo: 1 second is too long. There is no reason for such a long wait.
Why not just use default?:

select { case <-sub.Message(): t.Errorf("should not receive duplicate message") default: }

I've decreased the delay. It's required because the publish is asynchronous.

islamaliev · 2024-06-17T21:00:53Z

event/buffered_bus_test.go

+	assert.Equal(t, msg2, output2Ch2)
+}
+
+func TestBufferedBusSubscribersDontRecieveItemsAfterUnsubscribing(t *testing.T) {


praise: tests overall look good. Thanks for taking your time.

islamaliev

LGTM! Thanks Keenan for the adjustments.

shahzadlone

Thanks for testing the requested paths.

LGTM

nasdf added 7 commits June 5, 2024 16:00

wip

b3f45bc

move event names and types to event package

10c522b

refactor net events

8f870bd

add event messages. use separate event bus for user subscriptions

138bb84

update tests with event bus

144b392

Merge branch 'develop' into nasdf/refactor/events

243775a

merge push log and dag merge events

6b8d97e

nasdf added refactor This issue specific to or requires *notable* refactoring of existing codebases and components area/network Related to internal/external network components labels Jun 7, 2024

nasdf added this to the DefraDB v0.12 milestone Jun 7, 2024

nasdf self-assigned this Jun 7, 2024

nasdf requested a review from a team June 7, 2024 20:28

fredcarle reviewed Jun 7, 2024

View reviewed changes

tests/integration/events/utils.go Outdated Show resolved Hide resolved

fredcarle reviewed Jun 7, 2024

View reviewed changes

events/bus.go Outdated Show resolved Hide resolved

fredcarle reviewed Jun 7, 2024

View reviewed changes

events/bus.go Outdated Show resolved Hide resolved

fredcarle reviewed Jun 7, 2024

View reviewed changes

AndrewSisley reviewed Jun 10, 2024

View reviewed changes

tests/integration/p2p.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Jun 10, 2024

View reviewed changes

tests/integration/utils2.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Jun 10, 2024

View reviewed changes

tests/integration/utils2.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Jun 10, 2024

View reviewed changes

tests/integration/p2p.go Outdated Show resolved Hide resolved

AndrewSisley requested changes Jun 10, 2024

View reviewed changes

tests/integration/state.go Outdated Show resolved Hide resolved

events/bus.go Outdated Show resolved Hide resolved

fredcarle reviewed Jun 10, 2024

View reviewed changes

islamaliev reviewed Jun 10, 2024

View reviewed changes

events/bus.go Outdated Show resolved Hide resolved

events/bus.go Outdated Show resolved Hide resolved

events/bus.go Outdated Show resolved Hide resolved

events/bus.go Outdated Show resolved Hide resolved

events/bus_test.go Outdated Show resolved Hide resolved

islamaliev reviewed Jun 10, 2024

View reviewed changes

tests/integration/events/utils.go Outdated Show resolved Hide resolved

nasdf added 4 commits June 11, 2024 08:55

Merge branch 'develop' into nasdf/refactor/events

05f1579

rename events package to event

98e77d4

add event bus timeout

2da9153

add waitForMerge test util

3725fca

nasdf added 5 commits June 12, 2024 17:05

revert p2p waitForMerge change. add more info to cli peer info panic

311a37d

fix node constructor

cfa0ef9

skip waitForMerge when no waitForSync present

0357385

revert skipWaitForMerge. use peer info from state

3513390

test db merge sequential

184f0cc

nasdf requested a review from islamaliev June 13, 2024 17:56

islamaliev reviewed Jun 13, 2024

View reviewed changes

nasdf mentioned this pull request Jun 13, 2024

Merge retries can result in incorrect state #2718

Closed

shahzadlone reviewed Jun 17, 2024

View reviewed changes

nasdf added 2 commits June 17, 2024 10:50

Merge branch 'develop' into nasdf/refactor/events

38e0c48

add mutex to buffered bus

55aac76

nasdf requested a review from islamaliev June 17, 2024 18:04

nasdf added 2 commits June 17, 2024 12:14

document bus mutex

e6128ef

add buffered bus tests

801354c

islamaliev reviewed Jun 17, 2024

View reviewed changes

nasdf added 3 commits June 17, 2024 14:39

fix buffered bus test names

2dfa4d2

remove event.Bus interface

56136c5

make mocks

726be78

nasdf requested a review from islamaliev June 17, 2024 21:55

nasdf and others added 4 commits June 17, 2024 14:55

Merge branch 'develop' into nasdf/refactor/events

61ab97f

Merge branch 'develop' into nasdf/refactor/events

9fb2eeb

Merge branch 'develop' into nasdf/refactor/events

72f44ce

update test names

d0ad720

islamaliev approved these changes Jun 19, 2024

View reviewed changes

shahzadlone approved these changes Jun 19, 2024

View reviewed changes

nasdf and others added 2 commits June 20, 2024 09:25

Merge branch 'develop' into nasdf/refactor/events

aeec15d

use session for dag sync

7680133

nasdf merged commit 3856b80 into sourcenetwork:develop Jun 20, 2024
38 of 39 checks passed

islamaliev mentioned this pull request Jun 25, 2024

refactor: Decouple client.DB from net #2768

Merged

4 tasks

refactor: Use events to test network logic #2700

refactor: Use events to test network logic #2700

Conversation

nasdf commented Jun 7, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

fredcarle left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

AndrewSisley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

AndrewSisley left a comment •

edited

Loading

AndrewSisley Jun 17, 2024 •

edited

Loading