Improve hub locking #1694

MariemBaccari · 2023-10-03T06:46:47Z

This PR aims to:

Upgrade the go backend to go 1.21
Fix a small bug that caused double connections between the client and the server (because of the servers sharing twice the same peers in the greet lao if they have a double connection between each other)
Provide new data structures that allow finer-grain locking of the hub. The hub is currently entirely locked very often for processing messages which can be avoided by replacing the maps by thread-safe structs.
Creates new package "hub_state" to store the structures that hold the hub's state to improve the main file's readability

jbsv

Great job. A few questions though.

be1-go/hub/standard_hub/hub_state/ids_by_channel.go

be1-go/hub/standard_hub/mod.go

pierluca

I've reviewed about half of it, I still have a few files to go through, but here's the first batch of comments :-)

be1-go/channel/lao/mod.go

be1-go/hub/standard_hub/hub_state/channels_by_id.go

be1-go/hub/standard_hub/hub_state/ids_by_channel.go

be1-go/hub/standard_hub/hub_state/channels_by_id.go

be1-go/hub/standard_hub/hub_state/peers.go

be1-go/hub/standard_hub/hub_state/queries.go

be1-go/hub/standard_hub/message_handling.go

be1-go/hub/standard_hub/mod.go

be1-go/hub/standard_hub/mod_test.go

pierluca

Much better, and it's allowed me to spot a few other issues for future PRs :)
We're on the right path !

be1-go/hub/standard_hub/hub_state/ThreadSafeMap.go

be1-go/hub/standard_hub/mod.go

pierluca · 2023-10-26T08:17:27Z

be1-go/hub/standard_hub/hub_state/queries.go

+	}
+
+	if currentState {
+		return xerrors.Errorf("query with id %d already answered", id)


Just realized something, completely outside the scope of this PR.
This error propagates all the way to the socket.
So if another peer (for whatever reason) sends the "answer" message twice, they will get an error in return saying "we've already gotten your answer".
Is this something that would need to be shared with the other peer, or just logged ?
What would the other peer be expected to do - retry ? That would likely make things worse (infinite loop of errors / retries)
It seems manageable (in this case) to implement some degree of idempotency.

I agree that we should just log it because we wouldn't expect the peer to do anything. I think it could be a nice first issue so I'll add it to the repo (#1709). Regarding idempotency, would that also apply to when we try to create a channel that was already created ? Currently we send a "duplicate resource" error but how do we differentiate between cases that warrant idempotency and those that don't ?

Unrelated to that but I also noticed that we don't always respect the protocol when it comes to sending errors back. Since we have many "levels" of returning in go, the nature of the error gets lost by the time it arrives to the method that's supposed to create the error to send back so we don't have the right code. Thankfully, we already have methods designed to create the right error with the right code but those aren't always used for some reason. I'll investigate the individual cases and open github issues for them. They're easy fixes but also a good way to get to know the codebase so they'll be good first issues as well.

be1-go/hub/standard_hub/hub_state/queries.go

be1-go/hub/standard_hub/hub_state/peers.go

be1-go/channel/lao/mod.go

be1-go/hub/standard_hub/hub_state/MessageIds.go

sonarcloud · 2023-11-04T22:28:01Z

[PoP - PoPCHA-Web-Client] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-11-04T22:30:04Z

[PoP - Be1-Go] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

88.1% Coverage
0.0% Duplication

sonarcloud · 2023-11-04T22:31:33Z

[PoP - Be2-Scala] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-11-04T22:33:51Z

[PoP - Fe2-Android] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-11-04T22:37:23Z

[PoP - Fe1-Web] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

pierluca

As discussed, there are still some performance improvements possible, but this is already so much better than in the past.
Great job !

MariemBaccari added 2 commits October 3, 2023 07:32

Upgrade go version and go mod tidy

ea2bdbe

Add message ids thread safe structure

7442560

MariemBaccari requested a review from a team as a code owner October 3, 2023 06:46

MariemBaccari marked this pull request as draft October 3, 2023 06:46

MariemBaccari changed the title ~~Work be1 mariembaccari fine grain locking hub~~ Improve hub locking Oct 3, 2023

MariemBaccari added 5 commits October 3, 2023 09:43

fix hub tests

66155dc

Add peers struct

0e11844

fix queries data structure

3205c9e

fix inbox tests

5c912bb

debugging

f15fd57

MariemBaccari added the be1-go label Oct 10, 2023

MariemBaccari linked an issue Oct 10, 2023 that may be closed by this pull request

[BUG] Duplicate resource error when two servers are connected to each other #1697

Closed

6 tasks

MariemBaccari added 4 commits October 10, 2023 22:49

Add hub_state

864ec8c

remove additional config files

a45c2d9

fix typo

25977d0

Add documentation and fix CI

f853897

MariemBaccari marked this pull request as ready for review October 10, 2023 21:30

MariemBaccari added the decentralized Decentralized Communication Project label Oct 11, 2023

jbsv reviewed Oct 11, 2023

View reviewed changes

be1-go/hub/standard_hub/hub_state/ids_by_channel.go Outdated Show resolved Hide resolved

be1-go/hub/standard_hub/mod.go Show resolved Hide resolved

be1-go/hub/standard_hub/mod.go Outdated Show resolved Hide resolved

fix hub log

2c6af8e

MariemBaccari requested a review from pierluca October 11, 2023 12:34

pierluca reviewed Oct 11, 2023

View reviewed changes

pierluca requested changes Oct 12, 2023

View reviewed changes

MariemBaccari added 7 commits October 14, 2023 14:29

Add thread safe map

feecb98

Fix threadsafemap usage

55bf030

Add usage of sets

47a176c

Address Pierluca's comments

87e6a88

Add imports and foreach channels

70eb68a

Change go version in workflows

e0ae6a9

change go version on karate workflow

de9f7e4

MariemBaccari added 6 commits October 18, 2023 16:30

experimenting with CI and goimports

1314dbd

experimenting with CI and goimports

962663f

try to add goimports

87934ac

remove goimports

05a8a8b

fix documentation

54d4b75

cancel config changes

dd78bc2

MariemBaccari requested a review from pierluca October 24, 2023 12:00

pierluca reviewed Oct 26, 2023

View reviewed changes

MariemBaccari added 2 commits October 30, 2023 15:53

adress pierluca's comments

2bc39db

fix small typo

8b7482d

This was referenced Nov 4, 2023

O(n) accesses optimization #1708

Open

[BUG] Idempotency when receiving the answer to a query that has already been answered #1709

Closed

fix filenames

94aba08

MariemBaccari requested a review from pierluca November 6, 2023 14:49

pierluca approved these changes Nov 7, 2023

View reviewed changes

pierluca merged commit f325a91 into master Nov 7, 2023
18 checks passed

pierluca deleted the work-be1-mariembaccari-fine-grain-locking-hub branch November 7, 2023 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve hub locking #1694

Improve hub locking #1694

MariemBaccari commented Oct 3, 2023 •

edited

Loading

jbsv left a comment

pierluca left a comment

pierluca left a comment

pierluca Oct 26, 2023

MariemBaccari Oct 30, 2023 •

edited

Loading

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

pierluca left a comment

Improve hub locking #1694

Improve hub locking #1694

Conversation

MariemBaccari commented Oct 3, 2023 • edited Loading

jbsv left a comment

Choose a reason for hiding this comment

pierluca left a comment

Choose a reason for hiding this comment

pierluca left a comment

Choose a reason for hiding this comment

pierluca Oct 26, 2023

Choose a reason for hiding this comment

MariemBaccari Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

sonarcloud bot commented Nov 4, 2023

pierluca left a comment

Choose a reason for hiding this comment

MariemBaccari commented Oct 3, 2023 •

edited

Loading

MariemBaccari Oct 30, 2023 •

edited

Loading