Review comments

federationsender/statistics/statistics.go

@@ -20,8 +20,8 @@ type Statistics struct {
 	mutex   sync.RWMutex
 
 	// How many times should we tolerate consecutive failures before we
-	// just blacklist the host altogether? Bear in mind that the backoff
+	// just blacklist the host altogether? The backoff is exponential,
-	// is exponential, so the max time here to attempt is 2**failures.
+	// so the max time here to attempt is 2**failures seconds.
 	FailuresUntilBlacklist uint32
 }
 
@@ -77,10 +77,9 @@ type ServerStatistics struct {
 func (s *ServerStatistics) Success() {
 	s.successCounter.Add(1)
 	s.failCounter.Store(0)
+	s.blacklisted.Store(false)
 	if err := s.statistics.DB.RemoveServerFromBlacklist(s.serverName); err != nil {
 		logrus.WithError(err).Errorf("Failed to remove %q from blacklist", s.serverName)
-	} else {
-		s.blacklisted.Store(false)
 	}
 }
 
@@ -98,10 +97,9 @@ func (s *ServerStatistics) Failure() bool {
 		// to back off, which is probably in the region of hours by
 		// now. Mark the host as blacklisted and tell the caller to
 		// give up.
+		s.blacklisted.Store(true)
 		if err := s.statistics.DB.AddServerToBlacklist(s.serverName); err != nil {
 			logrus.WithError(err).Errorf("Failed to add %q to blacklist", s.serverName)
-		} else {
-			s.blacklisted.Store(true)
 		}
 		return true
 	}

federationsender/storage/postgres/blacklist_table.go

@@ -1,5 +1,4 @@
-// Copyright 2017-2018 New Vector Ltd
+// Copyright 2020 The Matrix.org Foundation C.I.C.
-// Copyright 2019-2020 The Matrix.org Foundation C.I.C.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -94,6 +93,9 @@ func (s *blacklistStatements) SelectBlacklist(
 		return false, err
 	}
 	defer res.Close() // nolint:errcheck
+	// The query will return the server name if the server is blacklisted, and
+	// will return no rows if not. By calling Next, we find out if a row was
+	// returned or not - we don't care about the value itself.
 	return res.Next(), nil
 }
 

federationsender/storage/sqlite3/blacklist_table.go

@@ -1,5 +1,4 @@
-// Copyright 2017-2018 New Vector Ltd
+// Copyright 2020 The Matrix.org Foundation C.I.C.
-// Copyright 2019-2020 The Matrix.org Foundation C.I.C.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -94,6 +93,9 @@ func (s *blacklistStatements) SelectBlacklist(
 		return false, err
 	}
 	defer res.Close() // nolint:errcheck
+	// The query will return the server name if the server is blacklisted, and
+	// will return no rows if not. By calling Next, we find out if a row was
+	// returned or not - we don't care about the value itself.
 	return res.Next(), nil
 }