Don't mutate inputs in DiffTopology or Topology.DiffHostlist.

Also guard against a similar race in `compareHosts`.

GODRIVER-1301

Author: bartle-stripe

x/mongo/driver/description/topology.go

@@ -7,9 +7,6 @@
 package description
 
 import (
-	"sort"
-	"strings"
-
 	"go.mongodb.org/mongo-driver/x/mongo/driver/address"
 )
 
@@ -41,39 +38,24 @@ type TopologyDiff struct {
 func DiffTopology(old, new Topology) TopologyDiff {
 	var diff TopologyDiff
 
-	// TODO: do this without sorting...
-	oldServers := serverSorter(old.Servers)
-	newServers := serverSorter(new.Servers)
-
-	sort.Sort(oldServers)
-	sort.Sort(newServers)
-
-	i := 0
-	j := 0
-	for {
-		if i < len(oldServers) && j < len(newServers) {
-			comp := strings.Compare(oldServers[i].Addr.String(), newServers[j].Addr.String())
-			switch comp {
-			case 1:
-				//left is bigger than
-				diff.Added = append(diff.Added, newServers[j])
-				j++
-			case -1:
-				// right is bigger
-				diff.Removed = append(diff.Removed, oldServers[i])
-				i++
-			case 0:
-				i++
-				j++
-			}
-		} else if i < len(oldServers) {
-			diff.Removed = append(diff.Removed, oldServers[i])
-			i++
-		} else if j < len(newServers) {
-			diff.Added = append(diff.Added, newServers[j])
-			j++
+	oldServers := make(map[string]bool)
+	for _, s := range old.Servers {
+		oldServers[s.Addr.String()] = true
+	}
+
+	for _, s := range new.Servers {
+		addr := s.Addr.String()
+		if oldServers[addr] {
+			delete(oldServers, addr)
 		} else {
-			break
+			diff.Added = append(diff.Added, s)
+		}
+	}
+
+	for _, s := range old.Servers {
+		addr := s.Addr.String()
+		if oldServers[addr] {
+			diff.Removed = append(diff.Removed, s)
 		}
 	}
 
@@ -90,47 +72,22 @@ type HostlistDiff struct {
 func (t Topology) DiffHostlist(hostlist []string) HostlistDiff {
 	var diff HostlistDiff
 
-	oldServers := serverSorter(t.Servers)
-	sort.Sort(oldServers)
-	sort.Strings(hostlist)
-
-	i := 0
-	j := 0
-	for {
-		if i < len(oldServers) && j < len(hostlist) {
-			oldServer := oldServers[i].Addr.String()
-			comp := strings.Compare(oldServer, hostlist[j])
-			switch comp {
-			case 1:
-				// oldServers[i] is bigger
-				diff.Added = append(diff.Added, hostlist[j])
-				j++
-			case -1:
-				// hostlist[j] is bigger
-				diff.Removed = append(diff.Removed, oldServer)
-				i++
-			case 0:
-				i++
-				j++
-			}
-		} else if i < len(oldServers) {
-			diff.Removed = append(diff.Removed, oldServers[i].Addr.String())
-			i++
-		} else if j < len(hostlist) {
-			diff.Added = append(diff.Added, hostlist[j])
-			j++
+	oldServers := make(map[string]bool)
+	for _, s := range t.Servers {
+		oldServers[s.Addr.String()] = true
+	}
+
+	for _, addr := range hostlist {
+		if oldServers[addr] {
+			delete(oldServers, addr)
 		} else {
-			break
+			diff.Added = append(diff.Added, addr)
 		}
 	}
 
-	return diff
-}
-
-type serverSorter []Server
+	for addr := range oldServers {
+		diff.Removed = append(diff.Removed, addr)
+	}
 
-func (ss serverSorter) Len() int      { return len(ss) }
-func (ss serverSorter) Swap(i, j int) { ss[i], ss[j] = ss[j], ss[i] }
-func (ss serverSorter) Less(i, j int) bool {
-	return strings.Compare(ss[i].Addr.String(), ss[j].Addr.String()) < 0
+	return diff
 }

x/mongo/driver/description/topology_test.go

@@ -0,0 +1,65 @@
+// Copyright (C) MongoDB, Inc. 2019-present.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License. You may obtain
+// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+package description
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDiffTopology(t *testing.T) {
+	s1 := Server{Addr: "1.0.0.0:27017"}
+	s2 := Server{Addr: "2.0.0.0:27017"}
+	s3 := Server{Addr: "3.0.0.0:27017"}
+	s4 := Server{Addr: "4.0.0.0:27017"}
+	s5 := Server{Addr: "5.0.0.0:27017"}
+	s6 := Server{Addr: "6.0.0.0:27017"}
+
+	t1 := Topology{
+		Servers: []Server{s6, s1, s3, s2},
+	}
+	t2 := Topology{
+		Servers: []Server{s2, s4, s3, s5},
+	}
+
+	diff := DiffTopology(t1, t2)
+
+	assert.ElementsMatch(t, []Server{s4, s5}, diff.Added)
+	assert.ElementsMatch(t, []Server{s1, s6}, diff.Removed)
+
+	// Ensure that original topology servers were not reordered.
+	assert.EqualValues(t, []Server{s6, s1, s3, s2}, t1.Servers)
+	assert.EqualValues(t, []Server{s2, s4, s3, s5}, t2.Servers)
+}
+
+func TestTopology_DiffHostlist(t *testing.T) {
+	h1 := "1.0.0.0:27017"
+	h2 := "2.0.0.0:27017"
+	h3 := "3.0.0.0:27017"
+	h4 := "4.0.0.0:27017"
+	h5 := "5.0.0.0:27017"
+	h6 := "6.0.0.0:27017"
+	s1 := Server{Addr: "1.0.0.0:27017"}
+	s2 := Server{Addr: "2.0.0.0:27017"}
+	s3 := Server{Addr: "3.0.0.0:27017"}
+	s6 := Server{Addr: "6.0.0.0:27017"}
+
+	topo := Topology{
+		Servers: []Server{s6, s1, s3, s2},
+	}
+	hostlist := []string{h2, h4, h3, h5}
+
+	diff := topo.DiffHostlist(hostlist)
+
+	assert.ElementsMatch(t, []string{h4, h5}, diff.Added)
+	assert.ElementsMatch(t, []string{h1, h6}, diff.Removed)
+
+	// Ensure that original topology servers and hostlist were not reordered.
+	assert.EqualValues(t, []Server{s6, s1, s3, s2}, topo.Servers)
+	assert.EqualValues(t, []string{h2, h4, h3, h5}, hostlist)
+}

x/mongo/driver/topology/polling_srv_records_test.go

@@ -108,14 +108,17 @@ func compareHosts(t *testing.T, received []description.Server, expected []string
 		t.Fatalf("Number of hosts in topology does not match expected value. Got %v; want %v.", len(received), len(expected))
 	}
 
-	actual := serverSorter(received)
+	// Take a copy of servers so we don't risk a data race similar to GODRIVER-1301.
+	servers := make([]description.Server, len(received))
+	copy(servers, received)
+	actual := serverSorter(servers)
 	sort.Sort(actual)
 	sort.Strings(expected)
 
-	for i := range received {
-		if received[i].Addr.String() != expected[i] {
+	for i := range servers {
+		if servers[i].Addr.String() != expected[i] {
 			t.Errorf("Hosts in topology differ from expected values. Got %v; want %v.",
-				received[i].Addr.String(), expected[i])
+				servers[i].Addr.String(), expected[i])
 		}
 	}
 }