Skip to content

Commit

Permalink
Fix three data races in tests
Browse files Browse the repository at this point in the history
Trying to use a logger that sends to t.Log is a problem when goroutines can outlive the
main test goroutine. It can cause the test to panic if a write happens after the test
exits.

In general there is no reason to send output to t.Log because 'go test' writes output to
stdout. Fix the data race by removing the logger in the integration test. Output will now
go to stdout.

Also fixes access to a field that should be read atomicly.

Finally the third test requires a lock to access the nodes.
  • Loading branch information
dnephin committed Mar 24, 2021
1 parent e5c3004 commit 7628fe3
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 48 deletions.
3 changes: 2 additions & 1 deletion integ_test.go
Expand Up @@ -2,6 +2,7 @@ package memberlist

import (
"fmt"
"log"
"os"
"testing"
"time"
Expand Down Expand Up @@ -41,7 +42,7 @@ func TestMemberlist_Integ(t *testing.T) {
c.GossipInterval = 20 * time.Millisecond
c.PushPullInterval = 200 * time.Millisecond
c.SecretKey = secret
c.Logger = testLoggerWithName(t, c.Name)
c.Logger = log.New(os.Stderr, c.Name, log.LstdFlags)

if i == 0 {
c.Events = &ChannelEventDelegate{eventCh}
Expand Down
20 changes: 6 additions & 14 deletions memberlist_test.go
Expand Up @@ -46,10 +46,8 @@ func testConfigNet(tb testing.TB, network byte) *Config {
config.BindAddr = getBindAddrNet(network).String()
config.Name = config.BindAddr
config.BindPort = 0 // choose free port
if tb != nil {
config.Logger = testLoggerWithName(tb, config.Name)
}
config.RequireNodeNames = true
config.Logger = log.New(os.Stderr, config.Name, log.LstdFlags)
return config
}

Expand Down Expand Up @@ -186,7 +184,6 @@ func TestCreate_protocolVersion(t *testing.T) {
c := DefaultLANConfig()
c.BindAddr = getBindAddr().String()
c.ProtocolVersion = tc.version
c.Logger = testLogger(t)

m, err := Create(c)
if err == nil {
Expand Down Expand Up @@ -219,7 +216,6 @@ func TestCreate_secretKey(t *testing.T) {
c := DefaultLANConfig()
c.BindAddr = getBindAddr().String()
c.SecretKey = tc.key
c.Logger = testLogger(t)

m, err := Create(c)
if err == nil {
Expand All @@ -239,7 +235,6 @@ func TestCreate_secretKeyEmpty(t *testing.T) {
c := DefaultLANConfig()
c.BindAddr = getBindAddr().String()
c.SecretKey = make([]byte, 0)
c.Logger = testLogger(t)

m, err := Create(c)
require.NoError(t, err)
Expand All @@ -253,7 +248,6 @@ func TestCreate_secretKeyEmpty(t *testing.T) {
func TestCreate_keyringOnly(t *testing.T) {
c := DefaultLANConfig()
c.BindAddr = getBindAddr().String()
c.Logger = testLogger(t)

keyring, err := NewKeyring(nil, make([]byte, 16))
require.NoError(t, err)
Expand All @@ -271,7 +265,6 @@ func TestCreate_keyringOnly(t *testing.T) {
func TestCreate_keyringAndSecretKey(t *testing.T) {
c := DefaultLANConfig()
c.BindAddr = getBindAddr().String()
c.Logger = testLogger(t)

keyring, err := NewKeyring(nil, make([]byte, 16))
require.NoError(t, err)
Expand Down Expand Up @@ -1322,14 +1315,14 @@ func TestMemberlist_SendTo(t *testing.T) {

// Ensure we got the messages
if len(msgs1) != 1 {
t.Fatalf("should have 1 messages!")
t.Fatalf("expected 1 message, got %d", len(msgs1))
}
if !reflect.DeepEqual(msgs1[0], []byte("pong")) {
t.Fatalf("bad msg %v", msgs1[0])
}

if len(msgs2) != 1 {
t.Fatalf("should have 1 messages!")
t.Fatalf("should have 1 message, got %d", len(msgs2))
}
if !reflect.DeepEqual(msgs2[0], []byte("ping")) {
t.Fatalf("bad msg %v", msgs2[0])
Expand Down Expand Up @@ -1483,7 +1476,7 @@ func TestMemberlist_Join_IPv6(t *testing.T) {
c1.Name = "A"
c1.BindAddr = "[::1]"
c1.BindPort = 0 // choose free
c1.Logger = testLoggerWithName(t, c1.Name)
c1.Logger = log.New(os.Stderr, c1.Name, log.LstdFlags)

m1, err := Create(c1)
require.NoError(t, err)
Expand All @@ -1494,7 +1487,7 @@ func TestMemberlist_Join_IPv6(t *testing.T) {
c2.Name = "B"
c2.BindAddr = "[::1]"
c2.BindPort = 0 // choose free
c2.Logger = testLoggerWithName(t, c2.Name)
c2.Logger = log.New(os.Stderr, c2.Name, log.LstdFlags)

m2, err := Create(c2)
require.NoError(t, err)
Expand Down Expand Up @@ -1558,7 +1551,6 @@ func TestAdvertiseAddr(t *testing.T) {
c.BindAddr = bindAddr.String()
c.BindPort = bindPort
c.Name = c.BindAddr
c.Logger = testLoggerWithName(t, c.Name)

c.AdvertiseAddr = advertiseAddr.String()
c.AdvertisePort = advertisePort
Expand Down Expand Up @@ -1783,7 +1775,7 @@ func TestMemberlist_EncryptedGossipTransition(t *testing.T) {
// Set the gossip interval fast enough to get a reasonable test,
// but slow enough to avoid "sendto: operation not permitted"
conf.GossipInterval = 100 * time.Millisecond
conf.Logger = testLoggerWithName(t, shortName)
conf.Logger = log.New(os.Stderr, shortName, log.LstdFlags)

pretty[conf.Name] = shortName
return conf
Expand Down
1 change: 0 additions & 1 deletion net_test.go
Expand Up @@ -660,7 +660,6 @@ func TestEncryptDecryptState(t *testing.T) {
SecretKey: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15},
ProtocolVersion: ProtocolVersionMax,
}
config.Logger = testLogger(t)

m, err := Create(config)
if err != nil {
Expand Down
12 changes: 9 additions & 3 deletions state_test.go
Expand Up @@ -3,10 +3,13 @@ package memberlist
import (
"bytes"
"fmt"
"log"
"net"
"os"
"reflect"
"strconv"
"sync"
"sync/atomic"
"testing"
"time"

Expand All @@ -21,7 +24,7 @@ func HostMemberlist(host string, t *testing.T, f func(*Config)) *Memberlist {
c.Name = host
c.BindAddr = host
c.BindPort = 0 // choose a free port
c.Logger = testLoggerWithName(t, host)
c.Logger = log.New(os.Stderr, host, log.LstdFlags)
if f != nil {
f(c)
}
Expand Down Expand Up @@ -127,8 +130,8 @@ func TestMemberList_ProbeNode_Suspect(t *testing.T) {
time.Sleep(10 * time.Millisecond)

// One of the peers should have attempted an indirect probe.
if m2.sequenceNum != 1 && m3.sequenceNum != 1 {
t.Fatalf("bad seqnos %v, %v", m2.sequenceNum, m3.sequenceNum)
if s2, s3 := atomic.LoadUint32(&m2.sequenceNum), atomic.LoadUint32(&m3.sequenceNum); s2 != 1 && s3 != 1 {
t.Fatalf("bad seqnos, expected both to be 1: %v, %v", s2, s3)
}
}

Expand Down Expand Up @@ -785,9 +788,12 @@ func TestMemberList_ProbeNode_Awareness_MissedNack(t *testing.T) {
probeTime := time.Now().Sub(startProbe)

// Node should be reported suspect.

m1.nodeLock.Lock()
if n.State != StateSuspect {
t.Fatalf("expect node to be suspect")
}
m1.nodeLock.Unlock()

// Make sure we timed out approximately on time.
if probeTime > probeTimeMax {
Expand Down
4 changes: 0 additions & 4 deletions transport_test.go
Expand Up @@ -19,7 +19,6 @@ func TestTransport_Join(t *testing.T) {
c1 := DefaultLANConfig()
c1.Name = "node1"
c1.Transport = t1
c1.Logger = testLogger(t)
m1, err := Create(c1)
if err != nil {
t.Fatalf("err: %v", err)
Expand All @@ -31,7 +30,6 @@ func TestTransport_Join(t *testing.T) {
c2 := DefaultLANConfig()
c2.Name = "node2"
c2.Transport = net.NewTransport("node2")
c2.Logger = testLogger(t)
m2, err := Create(c2)
if err != nil {
t.Fatalf("err: %v", err)
Expand Down Expand Up @@ -67,7 +65,6 @@ func TestTransport_Send(t *testing.T) {
c1.Name = "node1"
c1.Transport = t1
c1.Delegate = d1
c1.Logger = testLogger(t)
m1, err := Create(c1)
if err != nil {
t.Fatalf("err: %v", err)
Expand All @@ -79,7 +76,6 @@ func TestTransport_Send(t *testing.T) {
c2 := DefaultLANConfig()
c2.Name = "node2"
c2.Transport = net.NewTransport("node2")
c2.Logger = testLogger(t)
m2, err := Create(c2)
if err != nil {
t.Fatalf("err: %v", err)
Expand Down
25 changes: 0 additions & 25 deletions z_test.go

This file was deleted.

0 comments on commit 7628fe3

Please sign in to comment.