From a18c19bbf28910833324c72d01ca594a06430711 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Tue, 2 Jan 2024 23:17:59 +0000 Subject: [PATCH] Fix Contact Retry Failure to Restart (#541) commit daea5128c00798903aca7c574edbebe916ed6b4a (HEAD -> post-stable-fixes, origin/post-stable-fixes) Author: Sarah Jamie Lewis Date: Tue Jan 2 12:45:39 2024 -0800 Fixup Connection Test to check reconnecting status commit 347ac3cf48da4964fa252dfa09c4029c2695c6a3 Author: Sarah Jamie Lewis Date: Tue Jan 2 12:33:31 2024 -0800 Fixup Formatting and Quality Script ineffassign and misspell are no longer compatible with previous go workflows and the latest versions do not work. Commenting for now with intent to replace with better tooling. commit d9ce7737cc40363a2d3de4ce7e7fd148d94e62d2 Author: Sarah Jamie Lewis Date: Tue Jan 2 12:24:33 2024 -0800 Fix Contact Retry Failure to Restart When toggling between connected and disconnected, the Contact Retry plugin could find itself in a state where the new event would never get requeued. Also: Make the unsigned nature of limit in GetMessage* Apis explicit. Reviewed-on: https://git.openprivacy.ca/cwtch.im/cwtch/pulls/541 Reviewed-by: Dan Ballard --- .drone.yml | 36 +++++++++++++++++++------------- app/plugins/contactRetry.go | 11 +++++----- app/plugins/contactRetry_test.go | 21 +++++++------------ go.mod | 1 + go.sum | 2 ++ peer/cwtch_peer.go | 4 ++-- peer/cwtchprofilestorage.go | 2 +- peer/profile_interface.go | 4 ++-- testing/quality.sh | 15 ++++++------- 9 files changed, 50 insertions(+), 46 deletions(-) diff --git a/.drone.yml b/.drone.yml index 75f1322..5bfea52 100644 --- a/.drone.yml +++ b/.drone.yml @@ -5,27 +5,29 @@ name: linux-test steps: - name: fetch - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go commands: - go install honnef.co/go/tools/cmd/staticcheck@latest - - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/master/tor/tor - - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/master/tor/torrc - - chmod a+x tor - - go get -u golang.org/x/lint/golint + - go install go.uber.org/nilaway/cmd/nilaway@latest + - wget https://git.openprivacy.ca/openprivacy/buildfiles/raw/branch/master/tor/tor-0.4.8.9-linux-x86_64.tar.gz -O tor.tar.gz + - tar -xzf tor.tar.gz + - chmod a+x Tor/tor + - export PATH=$PWD/Tor/:$PATH + - export LD_LIBRARY_PATH=$PWD/Tor/ + - tor --version - export GO111MODULE=on - - go mod vendor - name: quality - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go commands: - - staticcheck ./... + - ./testing/quality.sh - name: units-tests - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go @@ -33,28 +35,32 @@ steps: - export PATH=`pwd`:$PATH - sh testing/tests.sh - name: integ-test - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go commands: - - export PATH=`pwd`:$PATH + - export PATH=$PWD/Tor/:$PATH + - export LD_LIBRARY_PATH=$PWD/Tor/ + - tor --version - go test -timeout=30m -race -v cwtch.im/cwtch/testing/ - name: filesharing-integ-test - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go commands: - - export PATH=`pwd`:$PATH + - export PATH=$PWD/Tor/:$PATH + - export LD_LIBRARY_PATH=$PWD/Tor/ - go test -timeout=20m -race -v cwtch.im/cwtch/testing/filesharing - name: filesharing-autodownload-integ-test - image: golang:1.19.1 + image: golang:1.21.5 volumes: - name: deps path: /go commands: - - export PATH=`pwd`:$PATH + - export PATH=$PWD/Tor/:$PATH + - export LD_LIBRARY_PATH=$PWD/Tor/ - go test -timeout=20m -race -v cwtch.im/cwtch/testing/autodownload - name: notify-gogs image: openpriv/drone-gogs diff --git a/app/plugins/contactRetry.go b/app/plugins/contactRetry.go index 1cb0cab..74a3ef9 100644 --- a/app/plugins/contactRetry.go +++ b/app/plugins/contactRetry.go @@ -302,8 +302,7 @@ func (cr *contactRetry) run() { cr.authorizedPeers.Store(id, true) if c, ok := cr.connections.Load(id); ok { contact := c.(*contact) - if contact.state == connections.DISCONNECTED && !contact.queued { - + if contact.state == connections.DISCONNECTED { // prioritize connections made in the last week if time.Since(contact.lastSeen).Hours() < PriorityQueueTimeSinceQualifierHours { cr.priorityQueue.insert(contact) @@ -461,10 +460,10 @@ func (cr *contactRetry) addConnection(id string, state connections.ConnectionSta cr.connections.Store(id, p) return } else { - // we have rerequested this connnection. Force set the queued parameter to true. - p, _ := cr.connections.Load(id) - if !p.(*contact).queued { - p.(*contact).queued = true + // we have rerequested this connnection, probably via an explicit ask, update it's state + if c, ok := cr.connections.Load(id); ok { + contact := c.(*contact) + contact.state = state } } } diff --git a/app/plugins/contactRetry_test.go b/app/plugins/contactRetry_test.go index 946ced8..ab79c13 100644 --- a/app/plugins/contactRetry_test.go +++ b/app/plugins/contactRetry_test.go @@ -46,9 +46,11 @@ func TestContactRetryQueue(t *testing.T) { } } + // We should very quickly become connecting... + time.Sleep(time.Second) pinf, _ := cr.connections.Load(testOnion) - if pinf.(*contact).queued == false { - t.Fatalf("test connection should be queued, actually: %v", pinf.(*contact).queued) + if pinf.(*contact).state != 1 { + t.Fatalf("test connection should be in connecting after update, actually: %v", pinf.(*contact).state) } // Asset that "test" is authenticated @@ -63,19 +65,12 @@ func TestContactRetryQueue(t *testing.T) { // Publish an unrelated event to trigger the Plugin to go through a queuing cycle // If we didn't do this we would have to wait 30 seconds for a check-in bus.Publish(event.NewEvent(event.PeerStateChange, map[event.Field]string{event.RemotePeer: "test2", event.ConnectionState: "Disconnected"})) + bus.Publish(event.NewEvent(event.QueuePeerRequest, map[event.Field]string{event.RemotePeer: testOnion, event.LastSeen: time.Now().Format(time.RFC3339Nano)})) + time.Sleep(time.Second) - if pinf.(*contact).queued != false { - t.Fatalf("test connection should not be queued, actually: %v", pinf.(*contact).queued) - } - - // Publish a new peer request... - bus.Publish(event.NewEvent(event.QueuePeerRequest, map[event.Field]string{event.RemotePeer: testOnion})) - time.Sleep(time.Second) // yield for a second so the event can catch up... - - // Peer test should be forced to queue.... pinf, _ = cr.connections.Load(testOnion) - if pinf.(*contact).queued != true { - t.Fatalf("test connection should be forced to queue after new queue peer request") + if pinf.(*contact).state != 1 { + t.Fatalf("test connection should be in connecting after update, actually: %v", pinf.(*contact).state) } cr.Shutdown() diff --git a/go.mod b/go.mod index afcb490..7c7f8f3 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( require ( filippo.io/edwards25519 v1.0.0 // indirect git.openprivacy.ca/openprivacy/bine v0.0.5 // indirect + github.com/client9/misspell v0.3.4 // indirect github.com/google/go-cmp v0.5.8 // indirect github.com/gtank/merlin v0.1.1 // indirect github.com/mimoo/StrobeGo v0.0.0-20220103164710-9a04d6ca976b // indirect diff --git a/go.sum b/go.sum index a033e11..2893bb5 100644 --- a/go.sum +++ b/go.sum @@ -8,6 +8,8 @@ git.openprivacy.ca/openprivacy/connectivity v1.11.0 h1:roASjaFtQLu+HdH5fa2wx6F00 git.openprivacy.ca/openprivacy/connectivity v1.11.0/go.mod h1:OQO1+7OIz/jLxDrorEMzvZA6SEbpbDyLGpjoFqT3z1Y= git.openprivacy.ca/openprivacy/log v1.0.3 h1:E/PMm4LY+Q9s3aDpfySfEDq/vYQontlvNj/scrPaga0= git.openprivacy.ca/openprivacy/log v1.0.3/go.mod h1:gGYK8xHtndRLDymFtmjkG26GaMQNgyhioNS82m812Iw= +github.com/client9/misspell v0.3.4 h1:ta993UF76GwbvJcIo3Y68y/M3WxlpEHPWIGDkJYwzJI= +github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/peer/cwtch_peer.go b/peer/cwtch_peer.go index 3ecc838..7b9aa1d 100644 --- a/peer/cwtch_peer.go +++ b/peer/cwtch_peer.go @@ -94,7 +94,7 @@ func (cp *cwtchPeer) EnhancedImportBundle(importString string) string { return cp.ImportBundle(importString).Error() } -func (cp *cwtchPeer) EnhancedGetMessages(conversation int, index int, count int) string { +func (cp *cwtchPeer) EnhancedGetMessages(conversation int, index int, count uint) string { var emessages = make([]EnhancedMessage, count) messages, err := cp.GetMostRecentMessages(conversation, 0, index, count) @@ -888,7 +888,7 @@ func (cp *cwtchPeer) GetChannelMessageCount(conversation int, channel int) (int, } // GetMostRecentMessages returns a selection of messages, ordered by most recently inserted -func (cp *cwtchPeer) GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error) { +func (cp *cwtchPeer) GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error) { return cp.storage.GetMostRecentMessages(conversation, channel, offset, limit) } diff --git a/peer/cwtchprofilestorage.go b/peer/cwtchprofilestorage.go index 74496c8..37330e9 100644 --- a/peer/cwtchprofilestorage.go +++ b/peer/cwtchprofilestorage.go @@ -781,7 +781,7 @@ func (cps *CwtchProfileStorage) SearchMessages(conversation int, channel int, pa } // GetMostRecentMessages returns the most recent messages in a channel up to a given limit at a given offset -func (cps *CwtchProfileStorage) GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error) { +func (cps *CwtchProfileStorage) GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error) { channelID := ChannelID{Conversation: conversation, Channel: channel} cps.mutex.Lock() diff --git a/peer/profile_interface.go b/peer/profile_interface.go index 9d70504..08591b7 100644 --- a/peer/profile_interface.go +++ b/peer/profile_interface.go @@ -131,7 +131,7 @@ type CwtchPeer interface { GetChannelMessage(conversation int, channel int, id int) (string, model.Attributes, error) GetChannelMessageCount(conversation int, channel int) (int, error) GetChannelMessageByContentHash(conversation int, channel int, contenthash string) (int, error) - GetMostRecentMessages(conversation int, channel int, offset int, limit int) ([]model.ConversationMessage, error) + GetMostRecentMessages(conversation int, channel int, offset int, limit uint) ([]model.ConversationMessage, error) UpdateMessageAttribute(conversation int, channel int, id int, key string, value string) error SearchConversations(pattern string) string @@ -142,7 +142,7 @@ type CwtchPeer interface { EnhancedGetMessageByContentHash(conversation int, hash string) string // EnhancedGetMessages returns a set of json-encoded enhanced messages, suitable for rendering in a UI - EnhancedGetMessages(conversation int, index int, count int) string + EnhancedGetMessages(conversation int, index int, count uint) string // Server Token APIS // TODO move these to feature protected interfaces diff --git a/testing/quality.sh b/testing/quality.sh index 0827cb5..e8f071a 100755 --- a/testing/quality.sh +++ b/testing/quality.sh @@ -4,24 +4,25 @@ echo "Checking code quality (you want to see no output here)" echo "" echo "" -echo "Linting:" +echo "Running staticcheck..." staticcheck ./... -# In the future we should enable this by default. However, there are a few false positives that make this +# In the future we should remove include-pkgs. However, there are a few false positives in the overall go stdlib that make this # too noisy right now, specifically assigning nil to initialize slices (safe), and using go internal context channels assigned # nil (also safe). # We also have one file infinite_channel.go written in a way that static analysis cannot reason about easily. So it is explictly # ignored. -# nilaway -exclude-file-docstrings="nolint:nilaway" ./... +echo "Running nilaway..." +nilaway -include-pkgs="cwtch.im/cwtch,cwtch.im/tapir,git.openprivacy.ca/openprivacy/connectivity" -exclude-file-docstrings="nolint:nilaway" ./... echo "Time to format" gofmt -l -s -w . # ineffassign (https://github.com/gordonklaus/ineffassign) -echo "Checking for ineffectual assignment of errors (unchecked errors...)" -ineffassign ./.. +# echo "Checking for ineffectual assignment of errors (unchecked errors...)" +# ineffassign . # misspell (https://github.com/client9/misspell/cmd/misspell) -echo "Checking for misspelled words..." -misspell . | grep -v "testing/" | grep -v "vendor/" | grep -v "go.sum" | grep -v ".idea" +# echo "Checking for misspelled words..." +# misspell . | grep -v "testing/" | grep -v "vendor/" | grep -v "go.sum" | grep -v ".idea"