advertisement callback now returns the list of provided namespaces (#4)

* changed advertisement callback to return the provided namespaces

* added linting support

* can't refresh namespaces before discovery loop is started

* fixed log message

* removed incorrect comment

* RefreshNamespaces renamed Advertise

.golangci.yml

@@ -0,0 +1,116 @@
+run:
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  deadline: 5m
+
+  # list of build tags, all linters use it. Default is empty list.
+  build-tags:
+    - integration
+
+# all available settings of specific linters
+linters-settings:
+  govet:
+    # report about shadowed variables
+    check-shadowing: true
+    enable-all: true
+    disable:
+      - fieldalignment
+
+  goimports:
+    local-prefixes: github.com/athanorlabs/atomic-swap
+
+  maligned:
+    # print struct with more effective memory layout or not, false by default
+    suggest-new: true
+
+  dupl:
+    # tokens count to trigger issue, 150 by default
+    threshold: 100
+
+  misspell:
+    # Correct spellings using locale preferences for US or UK.
+    # Default is to use a neutral variety of English.
+    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
+
+  tagliatelle:
+    # Check the struct tag name case.
+    case:
+      rules:
+        # See https://github.com/ldez/tagliatelle for values and examples:
+        json: goCamel
+
+  gocritic:
+    # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks.
+    # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags".
+    enabled-tags:
+      - performance
+    settings: # settings passed to gocritic
+      captLocal: # must be valid enabled check name
+        paramsOnly: true
+      rangeValCopy:
+        sizeThreshold: 32
+
+linters:
+  enable:
+    - bidichk
+    - bodyclose
+    - depguard
+    - errcheck
+    - exportloopref
+    - goconst
+    - gocyclo
+    - gofmt
+    - goimports
+    - goprintffuncname
+    - gosec
+    - gosimple
+    - govet
+    - ineffassign
+    - lll
+    - megacheck
+    - misspell
+    - nolintlint
+    - predeclared
+    - revive
+    - staticcheck
+    - tagliatelle
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - usestdlibvars
+
+  fast: false
+
+issues:
+  # List of regexps of issue texts to exclude, empty list by default.
+  # But independently from this option we use default exclude patterns,
+  # it can be disabled by `exclude-use-default: false`. To list all
+  # excluded by default patterns execute `golangci-lint run --help`
+  #exclude:
+
+  # Excluding configuration per-path, per-linter, per-text and per-source
+  exclude-rules:
+    # Exclude some linters from running on tests files.
+    - path: _test\.go
+      linters:
+        - gocyclo
+        - errcheck
+        - dupl
+        - gosec
+        - ineffassign
+
+    - text: 'G204: Subprocess launched with variable'
+      linters:
+        - gosec
+
+  # Independently from option `exclude` we use default exclude patterns,
+  # it can be disabled by this option. To list all
+  # excluded by default patterns execute `golangci-lint run --help`.
+  # Default value for this option is true.
+  exclude-use-default: false
+
+  # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
+  max-per-linter: 0
+
+  # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
+  max-same-issues: 0

Makefile

@@ -0,0 +1,32 @@
+GOPATH ?= $(shell go env GOPATH)
+
+.PHONY: all
+all: format lint test
+
+.PHONY: lint-go
+lint-go: 
+	./scripts/install-lint.sh
+	${GOPATH}/bin/golangci-lint run
+
+.PHONY: lint-shell
+lint-shell:
+	shellcheck --source-path=.:scripts scripts/*.sh
+
+.PHONY: lint
+lint: lint-go lint-shell
+
+.PHONY: format-go
+format-go:
+	test -x $(GOPATH)/bin/goimports || go install golang.org/x/tools/cmd/goimports@latest
+	$(GOPATH)/bin/goimports -local github.com/athanorlabs/go-p2p-net -w .
+
+.PHONY: format-shell
+format-shell:
+	shfmt -w scripts/*.sh
+
+.PHONY: format
+format: format-go format-shell
+
+.PHONY: test
+test: 
+	go test ./... -v -timeout=5m -count=1

discovery.go

@@ -21,22 +21,28 @@ const (
 	defaultMaxPeers     = 50 // TODO: make this configurable
 )
 
-// ShouldAdvertiseFunc is the type for a function that returns whether we should
-// regularly advertise inside the advertisement loop.
-// If it returns false, we don't advertise until the next loop iteration.
-type ShouldAdvertiseFunc = func() bool
-
 type discovery struct {
-	ctx                 context.Context
-	dht                 *dual.DHT
-	h                   libp2phost.Host
-	rd                  *libp2prouting.RoutingDiscovery
-	advertiseCh         chan []string // signals to advertise
-	shouldAdvertiseFunc ShouldAdvertiseFunc
+	ctx                  context.Context
+	dht                  *dual.DHT
+	h                    libp2phost.Host
+	rd                   *libp2prouting.RoutingDiscovery
+	advertiseCh          chan struct{} // signals to advertise
+	advertisedNamespaces func() []string
+}
+
+// setAdvertisedNamespacesFunc sets the function used to query the list of
+// namespaces to be advertised on every cycle of the advertisement loop. In most
+// use cases, this function should always return the empty namespace ("") on top
+// of any additional namespaces that should be advertised.
+func (d *discovery) setAdvertisedNamespacesFunc(fn func() []string) {
+	d.advertisedNamespaces = fn
 }
 
-func (d *discovery) setShouldAdvertiseFunc(fn ShouldAdvertiseFunc) {
-	d.shouldAdvertiseFunc = fn
+func (d *discovery) getAdvertisedNamespaces() []string {
+	if d.advertisedNamespaces == nil {
+		return []string{""}
+	}
+	return d.advertisedNamespaces()
 }
 
 func (d *discovery) start() error {
@@ -59,61 +65,41 @@ func (d *discovery) stop() error {
 }
 
 func (d *discovery) advertiseLoop() {
-	var toAdvertise []string
-	ttl := d.advertise(toAdvertise)
+	ttl := time.Duration(0) // don't block on first loop iteration
 
 	for {
 		select {
-		case ta := <-d.advertiseCh:
-			toAdvertise = ta
-			ttl = d.advertise(toAdvertise)
-		case <-time.After(ttl):
-			// the DHT clears provider records (ie. who is advertising what content)
-			// every 24 hours.
-			// so, if we don't have any offers available for 24 hours, then we are
-			// no longer present in the DHT as a provider.
-			// otherwise, we'll be present, but no offers will be sent when peers
-			// query us.
-			//
-			// this function is set in net/swapnet/host.go SetHandler().
-			if d.shouldAdvertiseFunc != nil && !d.shouldAdvertiseFunc() {
-				continue
-			}
-
-			ttl = d.advertise(toAdvertise)
 		case <-d.ctx.Done():
 			return
+		case <-d.advertiseCh:
+			// we've been asked to publish advertisements immediately, presumably
+			// because a new namespace was added
+		case <-time.After(ttl):
+			// publish advertisements on regular interval
 		}
+
+		// The DHT clears provider records (ie. who is advertising what content)
+		// every 24 hours. So, if we don't advertise a namespace for 24 hours,
+		// then we are no longer present in the DHT under that namespace.
+		ttl = d.advertise(d.getAdvertisedNamespaces())
 	}
 }
 
-// advertise advertises that we provide XMR in the DHT.
-// note: we only advertise that we are an XMR provider, but we don't
-// advertise our specific offers.
-// to find what our offers are, peers need to send us a QueryRequest
-// over the query subprotocol.
-// the return value is the amount of time the caller should wait before
-// trying to advertise again.
-func (d *discovery) advertise(toAdvertise []string) time.Duration {
-	log.Debug("advertising in the DHT...")
+// advertise advertises the passed set of namespaces in the DHT.
+func (d *discovery) advertise(namespaces []string) time.Duration {
 	err := d.dht.Bootstrap(d.ctx)
 	if err != nil {
-		log.Warnf("failed to bootstrap DHT: err=%s", err)
-		return tryAdvertiseTimeout
-	}
-
-	_, err = d.rd.Advertise(d.ctx, "")
-	if err != nil {
-		log.Debugf("failed to advertise in the DHT: err=%s", err)
+		log.Warnf("failed to bootstrap DHT: %s", err)
 		return tryAdvertiseTimeout
 	}
 
-	for _, provides := range toAdvertise {
+	for _, provides := range namespaces {
 		_, err = d.rd.Advertise(d.ctx, provides)
 		if err != nil {
-			log.Debugf("failed to advertise in the DHT: err=%s", err)
+			log.Debugf("failed to advertise %q in the DHT: %s", provides, err)
 			return tryAdvertiseTimeout
 		}
+		log.Debugf("advertised %q in the DHT", provides)
 	}
 
 	return defaultAdvertiseTTL
@@ -165,26 +151,26 @@ func (d *discovery) findPeers(provides string, timeout time.Duration) ([]peer.ID
 				return peerIDs, nil
 			}
 			return peerIDs, ctx.Err()
-		case peer, ok := <-peerCh:
+		case peerAddr, ok := <-peerCh:
 			if !ok {
 				// channel was closed, no more peers to read
 				return peerIDs, nil
 			}
-			if peer.ID == ourPeerID {
+			if peerAddr.ID == ourPeerID {
 				continue
 			}
 
-			log.Debugf("found new peer via DHT: %s", peer)
-			peerIDs = append(peerIDs, peer.ID)
+			log.Debugf("found new peer via DHT: %s", peerAddr)
+			peerIDs = append(peerIDs, peerAddr.ID)
 
 			// found a peer, try to connect if we need more peers
 			if len(d.h.Network().Peers()) < defaultMaxPeers {
-				err = d.h.Connect(d.ctx, peer)
+				err = d.h.Connect(d.ctx, peerAddr)
 				if err != nil {
-					log.Debugf("failed to connect to discovered peer %s: %s", peer.ID, err)
+					log.Debugf("failed to connect to discovered peer %s: %s", peerAddr.ID, err)
 				}
 			} else {
-				d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL)
+				d.h.Peerstore().AddAddrs(peerAddr.ID, peerAddr.Addrs, peerstore.PermanentAddrTTL)
 			}
 		}
 	}

discovery_test.go

@@ -13,9 +13,11 @@ func TestHost_Discover(t *testing.T) {
 	ha := newHost(t, basicTestConfig(t))
 	err := ha.Start()
 	require.NoError(t, err)
+
 	hb := newHost(t, basicTestConfig(t))
 	err = hb.Start()
 	require.NoError(t, err)
+
 	hc := newHost(t, basicTestConfig(t))
 	err = hc.Start()
 	require.NoError(t, err)
@@ -31,13 +33,15 @@ func TestHost_Discover(t *testing.T) {
 	require.GreaterOrEqual(t, len(hb.h.Network().Peers()), 2)
 	require.GreaterOrEqual(t, len(hc.h.Network().Peers()), 1)
 
-	strs := []string{"test"}
-	ha.Advertise(strs)
-	hb.Advertise(strs)
-	hc.Advertise(strs)
+	providedNamesapces := func() []string {
+		return []string{"test"}
+	}
+	ha.SetAdvertisedNamespacesFunc(providedNamesapces)
+	hb.SetAdvertisedNamespacesFunc(providedNamesapces)
+	hc.SetAdvertisedNamespacesFunc(providedNamesapces)
 	time.Sleep(testAdvertisementSleepDuration)
 
-	peerIDs, err := hc.Discover(strs[0], time.Second)
+	peerIDs, err := hc.Discover("test", time.Second)
 	require.NoError(t, err)
 	require.GreaterOrEqual(t, len(peerIDs), 1)
 	require.NotEmpty(t, peerIDs[0])