Re-sort masquerades while iterating to always dial the best

fronted.go

@@ -312,35 +312,59 @@ func (f *fronted) RoundTripHijack(req *http.Request) (*http.Response, net.Conn,
 
 // Dial dials out using all available masquerades until one succeeds.
 func (f *fronted) dialAll(ctx context.Context) (net.Conn, MasqueradeInterface, func(bool) bool, error) {
-	conn, m, masqueradeGood, err := f.dialAllWith(ctx, f.masquerades)
-	return conn, m, masqueradeGood, err
-}
-
-func (f *fronted) dialAllWith(ctx context.Context, masquerades sortedMasquerades) (net.Conn, MasqueradeInterface, func(bool) bool, error) {
+	defer func(op ops.Op) { op.End() }(ops.Begin("dial_all"))
 	// never take more than a minute trying to find a dialer
 	ctx, cancel := context.WithTimeout(ctx, 1*time.Minute)
 	defer cancel()
 
-	masqueradesToTry := masquerades.sortedCopy()
+	triedMasquerades := make(map[MasqueradeInterface]bool)
+	masqueradesToTry := f.masquerades.sortedCopy()
 	totalMasquerades := len(masqueradesToTry)
 dialLoop:
-	for _, m := range masqueradesToTry {
+	// Loop through up to len(masqueradesToTry) times, trying each masquerade in turn.
+	// If the context is done, return an error.
+	for i := 0; i < totalMasquerades; i++ {
 		select {
 		case <-ctx.Done():
-			log.Debugf("Timed out dialing to %v with %v total masquerades", m, totalMasquerades)
+			log.Debugf("Timed out dialing with %v total masquerades", totalMasquerades)
 			break dialLoop
 		default:
 			// okay
+
+		}
+
+		m, err := f.masqueradeToTry(masqueradesToTry, triedMasquerades)
+		if err != nil {
+			log.Errorf("No masquerades left to try")
+			break dialLoop
 		}
 		conn, masqueradeGood, err := f.dialMasquerade(m)
 		if err == nil {
 			return conn, m, masqueradeGood, nil
 		}
+
+		// As we're looping through the masquerades, each check takes time. As that's happening,
+		// other goroutines may be successfully vetting new masquerades, which will change the
+		// sorting. We want to make sure we're always trying the best masquerades first.
+		masqueradesToTry = f.masquerades.sortedCopy()
+		totalMasquerades = len(masqueradesToTry)
+		triedMasquerades[m] = true
 	}
 
 	return nil, nil, nil, log.Errorf("could not dial any masquerade? tried %v", totalMasquerades)
 }
 
+func (f *fronted) masqueradeToTry(masquerades sortedMasquerades, triedMasquerades map[MasqueradeInterface]bool) (MasqueradeInterface, error) {
+	for _, m := range masquerades {
+		if triedMasquerades[m] {
+			continue
+		}
+		return m, nil
+	}
+	// This should be quite rare, as it means we've tried typically thousands of masquerades.
+	return nil, errors.New("no masquerades left to try")
+}
+
 func (f *fronted) dialMasquerade(m MasqueradeInterface) (net.Conn, func(bool) bool, error) {
 	log.Tracef("Dialing to %v", m)
 

fronted_test.go

@@ -901,14 +901,82 @@ func TestFindWorkingMasquerades(t *testing.T) {
 	}
 }
 
+func TestMasqueradeToTry(t *testing.T) {
+	min := time.Now().Add(-time.Minute)
+	hour := time.Now().Add(-time.Hour)
+	domain1 := newMockMasqueradeWithLastSuccess("domain1.com", "1.1.1.1", 0, true, min)
+	domain2 := newMockMasqueradeWithLastSuccess("domain2.com", "2.2.2.2", 0, true, hour)
+	tests := []struct {
+		name             string
+		masquerades      sortedMasquerades
+		triedMasquerades map[MasqueradeInterface]bool
+		expected         MasqueradeInterface
+	}{
+		{
+			name: "No tried masquerades",
+			masquerades: sortedMasquerades{
+				domain1,
+				domain2,
+			},
+			triedMasquerades: map[MasqueradeInterface]bool{},
+			expected:         domain1,
+		},
+		{
+			name: "Some tried masquerades",
+			masquerades: sortedMasquerades{
+				domain1,
+				domain2,
+			},
+			triedMasquerades: map[MasqueradeInterface]bool{
+				domain1: true,
+			},
+			expected: domain2,
+		},
+		{
+			name: "All masquerades tried",
+			masquerades: sortedMasquerades{
+				domain1,
+				domain2,
+			},
+			triedMasquerades: map[MasqueradeInterface]bool{
+				domain1: true,
+				domain2: true,
+			},
+			expected: nil,
+		},
+		{
+			name:             "Empty masquerades list",
+			masquerades:      sortedMasquerades{},
+			triedMasquerades: map[MasqueradeInterface]bool{},
+			expected:         nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := &fronted{}
+			masquerades := tt.masquerades.sortedCopy()
+			result := f.masqueradeToTry(masquerades, tt.triedMasquerades)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
 // Generate a mock of a MasqueradeInterface with a Dial method that can optionally
 // return an error after a specified number of milliseconds.
 func newMockMasquerade(domain string, ipAddress string, timeout time.Duration, passesCheck bool) *mockMasquerade {
+	return newMockMasqueradeWithLastSuccess(domain, ipAddress, timeout, passesCheck, time.Time{})
+}
+
+// Generate a mock of a MasqueradeInterface with a Dial method that can optionally
+// return an error after a specified number of milliseconds.
+func newMockMasqueradeWithLastSuccess(domain string, ipAddress string, timeout time.Duration, passesCheck bool, lastSucceededTime time.Time) *mockMasquerade {
 	return &mockMasquerade{
-		Domain:      domain,
-		IpAddress:   ipAddress,
-		timeout:     timeout,
-		passesCheck: passesCheck,
+		Domain:            domain,
+		IpAddress:         ipAddress,
+		timeout:           timeout,
+		passesCheck:       passesCheck,
+		lastSucceededTime: lastSucceededTime,
 	}
 }