Review comments, make FS API take list of servernames, dedupe them, break out of loop properly on success

federationsender/api/perform.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	commonHTTP "github.com/matrix-org/dendrite/common/http"
+	"github.com/matrix-org/dendrite/federationsender/types"
 	"github.com/matrix-org/gomatrixserverlib"
 	"github.com/opentracing/opentracing-go"
 )
@@ -43,10 +44,10 @@ func (h *httpFederationSenderInternalAPI) PerformDirectoryLookup(
 }
 
 type PerformJoinRequest struct {
-	RoomID     string                       `json:"room_id"`
+	RoomID      string                 `json:"room_id"`
-	UserID     string                       `json:"user_id"`
+	UserID      string                 `json:"user_id"`
-	ServerName gomatrixserverlib.ServerName `json:"server_name"`
+	ServerNames types.ServerNames      `json:"server_names"`
-	Content    map[string]interface{}       `json:"content"`
+	Content     map[string]interface{} `json:"content"`
 }
 
 type PerformJoinResponse struct {

federationsender/internal/perform.go

@@ -9,6 +9,8 @@ import (
 	"github.com/matrix-org/dendrite/federationsender/internal/perform"
 	"github.com/matrix-org/dendrite/roomserver/version"
 	"github.com/matrix-org/gomatrixserverlib"
+	"github.com/matrix-org/util"
+	"github.com/sirupsen/logrus"
 )
 
 // PerformLeaveRequest implements api.FederationSenderInternalAPI
@@ -42,91 +44,114 @@ func (r *FederationSenderInternalAPI) PerformJoin(
 		supportedVersions = append(supportedVersions, version)
 	}
 
-	// Try to perform a make_join using the information supplied in the
+	// Deduplicate the server names we were provided.
-	// request.
+	util.Unique(request.ServerNames)
-	respMakeJoin, err := r.federation.MakeJoin(
+
-		ctx,
+	// Try each server that we were provided until we land on one that
-		request.ServerName,
+	// successfully completes the make-join send-join dance.
-		request.RoomID,
+	succeeded := false
-		request.UserID,
+	for _, serverName := range request.ServerNames {
-		supportedVersions,
+		// Try to perform a make_join using the information supplied in the
-	)
+		// request.
-	if err != nil {
+		respMakeJoin, err := r.federation.MakeJoin(
-		// TODO: Check if the user was not allowed to join the room.
+			ctx,
-		return fmt.Errorf("r.federation.MakeJoin: %w", err)
+			serverName,
+			request.RoomID,
+			request.UserID,
+			supportedVersions,
+		)
+		if err != nil {
+			// TODO: Check if the user was not allowed to join the room.
+			return fmt.Errorf("r.federation.MakeJoin: %w", err)
+		}
+
+		// Set all the fields to be what they should be, this should be a no-op
+		// but it's possible that the remote server returned us something "odd"
+		respMakeJoin.JoinEvent.Type = gomatrixserverlib.MRoomMember
+		respMakeJoin.JoinEvent.Sender = request.UserID
+		respMakeJoin.JoinEvent.StateKey = &request.UserID
+		respMakeJoin.JoinEvent.RoomID = request.RoomID
+		respMakeJoin.JoinEvent.Redacts = ""
+		if request.Content == nil {
+			request.Content = map[string]interface{}{}
+		}
+		request.Content["membership"] = "join"
+		if err = respMakeJoin.JoinEvent.SetContent(request.Content); err != nil {
+			return fmt.Errorf("respMakeJoin.JoinEvent.SetContent: %w", err)
+		}
+		if err = respMakeJoin.JoinEvent.SetUnsigned(struct{}{}); err != nil {
+			return fmt.Errorf("respMakeJoin.JoinEvent.SetUnsigned: %w", err)
+		}
+
+		// Work out if we support the room version that has been supplied in
+		// the make_join response.
+		if respMakeJoin.RoomVersion == "" {
+			respMakeJoin.RoomVersion = gomatrixserverlib.RoomVersionV1
+		}
+		if _, err = respMakeJoin.RoomVersion.EventFormat(); err != nil {
+			return fmt.Errorf("respMakeJoin.RoomVersion.EventFormat: %w", err)
+		}
+
+		// Build the join event.
+		event, err := respMakeJoin.JoinEvent.Build(
+			time.Now(),
+			r.cfg.Matrix.ServerName,
+			r.cfg.Matrix.KeyID,
+			r.cfg.Matrix.PrivateKey,
+			respMakeJoin.RoomVersion,
+		)
+		if err != nil {
+			return fmt.Errorf("respMakeJoin.JoinEvent.Build: %w", err)
+		}
+
+		// Try to perform a send_join using the newly built event.
+		respSendJoin, err := r.federation.SendJoin(
+			ctx,
+			serverName,
+			event,
+			respMakeJoin.RoomVersion,
+		)
+		if err != nil {
+			logrus.WithError(err).Warnf("r.federation.SendJoin failed")
+			continue
+		}
+
+		// Check that the send_join response was valid.
+		joinCtx := perform.JoinContext(r.federation, r.keyRing)
+		if err = joinCtx.CheckSendJoinResponse(
+			ctx, event, serverName, respMakeJoin, respSendJoin,
+		); err != nil {
+			logrus.WithError(err).Warnf("joinCtx.CheckSendJoinResponse failed")
+			continue
+		}
+
+		// If we successfully performed a send_join above then the other
+		// server now thinks we're a part of the room. Send the newly
+		// returned state to the roomserver to update our local view.
+		if err = r.producer.SendEventWithState(
+			ctx,
+			respSendJoin.ToRespState(),
+			event.Headered(respMakeJoin.RoomVersion),
+		); err != nil {
+			logrus.WithError(err).Warnf("r.producer.SendEventWithState failed")
+			continue
+		}
+
+		// We're all good.
+		succeeded = true
+		break
 	}
 
-	// Set all the fields to be what they should be, this should be a no-op
+	if succeeded {
-	// but it's possible that the remote server returned us something "odd"
+		// Everything went to plan.
-	respMakeJoin.JoinEvent.Type = gomatrixserverlib.MRoomMember
+		return nil
-	respMakeJoin.JoinEvent.Sender = request.UserID
+	} else {
-	respMakeJoin.JoinEvent.StateKey = &request.UserID
+		// We didn't complete a join for some reason.
-	respMakeJoin.JoinEvent.RoomID = request.RoomID
+		return fmt.Errorf(
-	respMakeJoin.JoinEvent.Redacts = ""
+			"failed to join user %q to room %q through %d server(s)",
-	if request.Content == nil {
+			request.UserID, request.RoomID, len(request.ServerNames),
-		request.Content = map[string]interface{}{}
+		)
 	}
-	request.Content["membership"] = "join"
-	if err = respMakeJoin.JoinEvent.SetContent(request.Content); err != nil {
-		return fmt.Errorf("respMakeJoin.JoinEvent.SetContent: %w", err)
-	}
-	if err = respMakeJoin.JoinEvent.SetUnsigned(struct{}{}); err != nil {
-		return fmt.Errorf("respMakeJoin.JoinEvent.SetUnsigned: %w", err)
-	}
-
-	// Work out if we support the room version that has been supplied in
-	// the make_join response.
-	if respMakeJoin.RoomVersion == "" {
-		respMakeJoin.RoomVersion = gomatrixserverlib.RoomVersionV1
-	}
-	if _, err = respMakeJoin.RoomVersion.EventFormat(); err != nil {
-		return fmt.Errorf("respMakeJoin.RoomVersion.EventFormat: %w", err)
-	}
-
-	// Build the join event.
-	event, err := respMakeJoin.JoinEvent.Build(
-		time.Now(),
-		r.cfg.Matrix.ServerName,
-		r.cfg.Matrix.KeyID,
-		r.cfg.Matrix.PrivateKey,
-		respMakeJoin.RoomVersion,
-	)
-	if err != nil {
-		return fmt.Errorf("respMakeJoin.JoinEvent.Build: %w", err)
-	}
-
-	// Try to perform a send_join using the newly built event.
-	respSendJoin, err := r.federation.SendJoin(
-		ctx,
-		request.ServerName,
-		event,
-		respMakeJoin.RoomVersion,
-	)
-	if err != nil {
-		return fmt.Errorf("r.federation.SendJoin: %w", err)
-	}
-
-	// Check that the send_join response was valid.
-	joinCtx := perform.JoinContext(r.federation, r.keyRing)
-	if err = joinCtx.CheckSendJoinResponse(
-		ctx, event, request.ServerName, respMakeJoin, respSendJoin,
-	); err != nil {
-		return fmt.Errorf("perform.JoinRequest.CheckSendJoinResponse: %w", err)
-	}
-
-	// If we successfully performed a send_join above then the other
-	// server now thinks we're a part of the room. Send the newly
-	// returned state to the roomserver to update our local view.
-	if err = r.producer.SendEventWithState(
-		ctx,
-		respSendJoin.ToRespState(),
-		event.Headered(respMakeJoin.RoomVersion),
-	); err != nil {
-		return fmt.Errorf("r.producer.SendEventWithState: %w", err)
-	}
-
-	// Everything went to plan.
-	return nil
 }
 
 // PerformLeaveRequest implements api.FederationSenderInternalAPI

federationsender/types/types.go

@@ -28,6 +28,12 @@ type JoinedHost struct {
 	ServerName gomatrixserverlib.ServerName
 }
 
+type ServerNames []gomatrixserverlib.ServerName
+
+func (s ServerNames) Len() int           { return len(s) }
+func (s ServerNames) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
+func (s ServerNames) Less(i, j int) bool { return s[i] < s[j] }
+
 // A EventIDMismatchError indicates that we have got out of sync with the
 // room server.
 type EventIDMismatchError struct {

roomserver/internal/perform_join.go

@@ -179,35 +179,16 @@ func (r *RoomserverInternalAPI) performJoinRoomByID(
 		}
 
 		// Try joining by all of the supplied server names.
-		// TODO: Update the FS API so that it accepts a list of server names and
+		fedReq := fsAPI.PerformJoinRequest{
-		// does this bit by itself.
+			RoomID:      req.RoomIDOrAlias, // the room ID to try and join
-		joined := false
+			UserID:      req.UserID,        // the user ID joining the room
-		for _, serverName := range req.ServerNames {
+			ServerNames: req.ServerNames,   // the server to try joining with
-			// Otherwise, if we've reached this point, the room isn't a local room
+			Content:     req.Content,       // the membership event content
-			// and we should ask the federation sender to try and join for us.
-			fedReq := fsAPI.PerformJoinRequest{
-				RoomID:     req.RoomIDOrAlias, // the room ID to try and join
-				UserID:     req.UserID,        // the user ID joining the room
-				ServerName: serverName,        // the server to try joining with
-				Content:    req.Content,       // the membership event content
-			}
-			fedRes := fsAPI.PerformJoinResponse{}
-			err = r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes)
-			if err != nil {
-				logrus.WithError(err).Errorf("error joining federated room %q", req.RoomIDOrAlias)
-				continue
-			}
-			joined = true
-			break
 		}
-
+		fedRes := fsAPI.PerformJoinResponse{}
-		// If we didn't successfully join the room using any of the supplied
+		err = r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes)
-		// servers then return an error saying such.
+		if err != nil {
-		if !joined {
+			return fmt.Errorf("Error joining federated room: %q", err)
-			return fmt.Errorf(
-				"Failed to join %q using %d server(s)",
-				req.RoomIDOrAlias, len(req.ServerNames),
-			)
 		}
 
 	default: