Fixes from CR

all_channels_test.go

@@ -27,18 +27,18 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/uber/tchannel-go"
+	. "github.com/uber/tchannel-go"
 	"github.com/uber/tchannel-go/testutils"
 )
 
 func TestAllChannelsRegistered(t *testing.T) {
-	introspectOpts := &tchannel.IntrospectionOptions{IncludeOtherChannels: true}
+	introspectOpts := &IntrospectionOptions{IncludeOtherChannels: true}
 
-	ch1_1, err := tchannel.NewChannel("ch1", nil)
+	ch1_1, err := NewChannel("ch1", nil)
 	require.NoError(t, err, "Channel create failed")
-	ch1_2, err := tchannel.NewChannel("ch1", nil)
+	ch1_2, err := NewChannel("ch1", nil)
 	require.NoError(t, err, "Channel create failed")
-	ch2_1, err := tchannel.NewChannel("ch2", nil)
+	ch2_1, err := NewChannel("ch2", nil)
 	require.NoError(t, err, "Channel create failed")
 
 	state := ch1_1.IntrospectState(introspectOpts)
@@ -52,7 +52,7 @@ func TestAllChannelsRegistered(t *testing.T) {
 	assert.Equal(t, 0, len(state.OtherChannels["ch1"]))
 	assert.Equal(t, 1, len(state.OtherChannels["ch2"]))
 
-	ch2_2, err := tchannel.NewChannel("ch2", nil)
+	ch2_2, err := NewChannel("ch2", nil)
 
 	state = ch1_1.IntrospectState(introspectOpts)
 	require.NoError(t, err, "Channel create failed")

relay.go

@@ -76,7 +76,7 @@ type relayItems struct {
 	items  map[uint32]relayItem
 }
 
-func newRelayItems(wheel *timers.Wheel, logger Logger) *relayItems {
+func newRelayItems(logger Logger, wheel *timers.Wheel) *relayItems {
 	return &relayItems{
 		items:  make(map[uint32]relayItem),
 		logger: logger,
@@ -203,8 +203,8 @@ func NewRelayer(ch *Channel, conn *Connection) *Relayer {
 		timeoutTick:  ch.relayTimeoutTick,
 		wheel:        wheel,
 		localHandler: ch.relayLocal,
-		outbound:     newRelayItems(wheel, ch.Logger().WithFields(LogField{"relay", "outbound"})),
-		inbound:      newRelayItems(wheel, ch.Logger().WithFields(LogField{"relay", "inbound"})),
+		outbound:     newRelayItems(ch.Logger().WithFields(LogField{"relay", "outbound"}), wheel),
+		inbound:      newRelayItems(ch.Logger().WithFields(LogField{"relay", "inbound"}), wheel),
 		peers:        ch.rootPeers(),
 		conn:         conn,
 		logger:       conn.log,

timers/timers.go

@@ -165,11 +165,8 @@ func NewWheel(period, maxTimeout time.Duration) *Wheel {
 }
 
 func newWheel(period, maxTimeout time.Duration, clock clock.Clock) *Wheel {
-	tickNanos, power := nextPowerOfTwo(int64(period))
+	tickNanos, power := nextPowerOfTwo(int64(period)/2 + 1)
 	numBuckets, _ := nextPowerOfTwo(int64(maxTimeout) / tickNanos)
-	if time.Duration(numBuckets*tickNanos) < maxTimeout {
-		numBuckets = numBuckets << 1
-	}
 	w := &Wheel{
 		clock:     clock,
 		periodExp: power,

timers/timers_test.go

@@ -12,13 +12,30 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-// O(n), only for use in tests.
-func (t *Timer) len() int {
-	n := 0
-	for el := t; el != nil; el = el.next {
-		n++
+func assertListWellFormed(t testing.TB, list *Timer, expectedLen int) {
+	if expectedLen == 0 {
+		assert.Nil(t, list, "Expected zero-length list to be nil.")
+		return
 	}
-	return n
+	var (
+		last   *Timer
+		length int
+	)
+	head := list
+	seen := make(map[*Timer]struct{})
+	for el := list; el != nil; el = el.next {
+		if _, ok := seen[el]; ok {
+			t.Fatalf("Detected cycle in linked list with repeated element: %+v", *el)
+		}
+		seen[el] = struct{}{}
+		if el != head {
+			assert.Nil(t, el.tail, "Only head should have a tail set.")
+		}
+		last = el
+		length++
+	}
+	assert.Equal(t, head.tail, last, "Expected list tail to point to last element.")
+	assert.Equal(t, expectedLen, length, "Unexpected list length.")
 }
 
 func fakeWheel(tick time.Duration) (*Wheel, *clock.Mock) {
@@ -62,34 +79,28 @@ func TestTimersLinkedListPushOneNils(t *testing.T) {
 	var root *Timer
 	head := newTimer(0, nil)
 	root = root.pushOne(head)
-	assert.Equal(t, 1, root.len(), "Unexpected length after pushing one timer to nil root.")
+	assertListWellFormed(t, root, 1)
 	assert.Equal(t, head, root, "Expected pushOne with nil receiver to return head.")
-	assert.Nil(t, root.next, "Expected one-node list to have a nil next link.")
-	assert.Equal(t, root, root.tail, "Expected head of single-node list to be its own tail.")
 	assert.Panics(t, func() { root.pushOne(nil) }, "Expected pushOne'ing a nil to panic.")
 }
 
 func TestTimersLinkedListPushNils(t *testing.T) {
 	// nil receiver & head
 	var root *Timer
-	assert.Equal(t, 0, root.push(nil).len(), "Unexpected length after pushing nil on nil timer.")
+	assertListWellFormed(t, root.push(nil), 0)
 
 	// nil receiver
 	head := newTimer(0, nil)
 	root = root.push(head)
-	assert.Equal(t, 1, root.len(), "Unexpected length after pushing timer to nil root.")
+	assertListWellFormed(t, root, 1)
 	assert.Equal(t, head, root, "Expected push with nil receiver to return head list.")
-	assert.Nil(t, root.next, "Expected one-node list to have a nil next link.")
-	assert.Equal(t, root, root.tail, "Expected head of single-node list to be its own tail.")
 
 	// nil head
 	originalRoot := newTimer(0, nil)
 	root = originalRoot
 	root = root.push(nil)
-	assert.Equal(t, 1, root.len(), "Unexpected length after pushing nil to a len-1 root.")
+	assertListWellFormed(t, root, 1)
 	assert.Equal(t, originalRoot, root, "Expected pushing nil onto a list to return original list.")
-	assert.Nil(t, root.next, "Expected one-node list to have a nil next link.")
-	assert.Equal(t, root, root.tail, "Expected head of single-node list to be its own tail.")
 }
 
 func TestTimersLinkedList(t *testing.T) {
@@ -105,27 +116,19 @@ func TestTimersLinkedList(t *testing.T) {
 	var root *Timer
 	root = root.pushOne(els[0])
 	assert.Equal(t, root, els[0], "Unexpected first node.")
-	assert.Equal(t, 1, root.len(), "Unexpected length after pushing one element.")
-	assert.Nil(t, root.next, "Expected one-node list to have a nil next link.")
-	assert.Equal(t, els[0], root.tail, "Expected head of single-node list to be its own tail.")
+	assertListWellFormed(t, root, 1)
 
 	// Add a second element.
 	root = root.pushOne(els[1])
-	assert.Equal(t, root, els[1], "Expected new head to precede existing nodes.")
-	assert.Equal(t, 2, root.len(), "Expected pushing to list to extend length.")
-	assert.Equal(t, els[0], root.next, "Expected head node to point to next node.")
-	assert.Equal(t, els[0], root.tail, "Expected tail of two-node list to point to last node.")
+	assertListWellFormed(t, root, 2)
 
 	// Push a list.
 	root = root.push((*Timer)(nil).
 		pushOne(els[2]).
 		pushOne(els[3]).
 		pushOne(els[4]).
 		pushOne(els[5]))
-	assert.Equal(t, root, els[5], "Expected pushing a list to return new head.")
-	assert.Equal(t, 6, root.len(), "Expected pushing a list to extend length.")
-	assert.Equal(t, els[4], root.next, "Expected head of list to point to next node.")
-	assert.Equal(t, els[0], root.tail, "Expected tail of list to point to last node.")
+	assertListWellFormed(t, root, 6)
 
 	var deadlines []uint64
 	for el := root; el != nil; el = el.next {
@@ -194,9 +197,9 @@ func TestTimerBucketCalculations(t *testing.T) {
 	}{
 		// If everything is a power of two, buckets = max / tick.
 		{2, 8, 4},
-		// Tick gets bumped up to 1<<3, so we need 3 buckets to support 20ns
-		// max without overlap. We then bump that to 1<<2.
-		{6, 20, 4},
+		// Tick gets dropped to 1<<2, so we need 5 buckets to support 20ns
+		// max without overlap. We then bump that to 1<<3.
+		{6, 20, 8},
 	}
 
 	for _, tt := range tests {