From 119cde37668badb04b9ca9445cf1d87a880bcbd5 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 1 Aug 2022 15:29:19 +0100 Subject: [PATCH] De-race `types.RoomInfo` (#2600) --- roomserver/internal/helpers/auth.go | 4 +-- roomserver/internal/helpers/helpers.go | 2 +- roomserver/internal/perform/perform_admin.go | 2 +- .../internal/perform/perform_backfill.go | 6 ++-- .../internal/perform/perform_inbound_peek.go | 2 +- roomserver/internal/perform/perform_invite.go | 4 +-- roomserver/internal/query/query.go | 12 ++++---- roomserver/storage/postgres/rooms_table.go | 6 ++-- roomserver/storage/shared/room_updater.go | 4 +-- roomserver/storage/shared/storage.go | 12 ++++---- roomserver/storage/sqlite3/rooms_table.go | 6 ++-- roomserver/storage/tables/rooms_table_test.go | 24 +++++++-------- roomserver/types/types.go | 30 +++++++++++++++++-- 13 files changed, 72 insertions(+), 42 deletions(-) diff --git a/roomserver/internal/helpers/auth.go b/roomserver/internal/helpers/auth.go index 0229f822f..648c50cf6 100644 --- a/roomserver/internal/helpers/auth.go +++ b/roomserver/internal/helpers/auth.go @@ -50,14 +50,14 @@ func CheckForSoftFail( if err != nil { return false, fmt.Errorf("db.RoomNID: %w", err) } - if roomInfo == nil || roomInfo.IsStub { + if roomInfo == nil || roomInfo.IsStub() { return false, nil } // Then get the state entries for the current state snapshot. // We'll use this to check if the event is allowed right now. roomState := state.NewStateResolution(db, roomInfo) - authStateEntries, err = roomState.LoadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID) + authStateEntries, err = roomState.LoadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID()) if err != nil { return true, fmt.Errorf("roomState.LoadStateAtSnapshot: %w", err) } diff --git a/roomserver/internal/helpers/helpers.go b/roomserver/internal/helpers/helpers.go index 16a6f615d..d61aa08cb 100644 --- a/roomserver/internal/helpers/helpers.go +++ b/roomserver/internal/helpers/helpers.go @@ -409,7 +409,7 @@ func QueryLatestEventsAndState( if err != nil { return err } - if roomInfo == nil || roomInfo.IsStub { + if roomInfo == nil || roomInfo.IsStub() { response.RoomExists = false return nil } diff --git a/roomserver/internal/perform/perform_admin.go b/roomserver/internal/perform/perform_admin.go index 1cb52966a..6c7c6c98b 100644 --- a/roomserver/internal/perform/perform_admin.go +++ b/roomserver/internal/perform/perform_admin.go @@ -52,7 +52,7 @@ func (r *Admin) PerformAdminEvacuateRoom( } return } - if roomInfo == nil || roomInfo.IsStub { + if roomInfo == nil || roomInfo.IsStub() { res.Error = &api.PerformError{ Code: api.PerformErrorNoRoom, Msg: fmt.Sprintf("Room %s not found", req.RoomID), diff --git a/roomserver/internal/perform/perform_backfill.go b/roomserver/internal/perform/perform_backfill.go index 3f98fbc24..5b7ed22ee 100644 --- a/roomserver/internal/perform/perform_backfill.go +++ b/roomserver/internal/perform/perform_backfill.go @@ -73,7 +73,7 @@ func (r *Backfiller) PerformBackfill( if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return fmt.Errorf("PerformBackfill: missing room info for room %s", request.RoomID) } @@ -106,7 +106,7 @@ func (r *Backfiller) backfillViaFederation(ctx context.Context, req *api.Perform if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return fmt.Errorf("backfillViaFederation: missing room info for room %s", req.RoomID) } requester := newBackfillRequester(r.DB, r.FSAPI, r.ServerName, req.BackwardsExtremities, r.PreferServers) @@ -434,7 +434,7 @@ FindSuccessor: logrus.WithError(err).WithField("room_id", roomID).Error("ServersAtEvent: failed to get RoomInfo for room") return nil } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { logrus.WithField("room_id", roomID).Error("ServersAtEvent: failed to get RoomInfo for room, room is missing") return nil } diff --git a/roomserver/internal/perform/perform_inbound_peek.go b/roomserver/internal/perform/perform_inbound_peek.go index 32c81e849..29decd363 100644 --- a/roomserver/internal/perform/perform_inbound_peek.go +++ b/roomserver/internal/perform/perform_inbound_peek.go @@ -50,7 +50,7 @@ func (r *InboundPeeker) PerformInboundPeek( if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return nil } response.RoomExists = true diff --git a/roomserver/internal/perform/perform_invite.go b/roomserver/internal/perform/perform_invite.go index fce529305..e1ff4eabb 100644 --- a/roomserver/internal/perform/perform_invite.go +++ b/roomserver/internal/perform/perform_invite.go @@ -125,7 +125,7 @@ func (r *Inviter) PerformInvite( return outputUpdates, nil } - if (info == nil || info.IsStub) && !isOriginLocal && isTargetLocal { + if (info == nil || info.IsStub()) && !isOriginLocal && isTargetLocal { // The invite came in over federation for a room that we don't know about // yet. We need to handle this a bit differently to most invites because // we don't know the room state, therefore the roomserver can't process @@ -276,7 +276,7 @@ func buildInviteStrippedState( } roomState := state.NewStateResolution(db, info) stateEntries, err := roomState.LoadStateAtSnapshotForStringTuples( - ctx, info.StateSnapshotNID, stateWanted, + ctx, info.StateSnapshotNID(), stateWanted, ) if err != nil { return nil, err diff --git a/roomserver/internal/query/query.go b/roomserver/internal/query/query.go index f652a8ea3..6d7c6e6bb 100644 --- a/roomserver/internal/query/query.go +++ b/roomserver/internal/query/query.go @@ -61,7 +61,7 @@ func (r *Queryer) QueryStateAfterEvents( if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return nil } @@ -302,7 +302,7 @@ func (r *Queryer) QueryServerJoinedToRoom( if err != nil { return fmt.Errorf("r.DB.RoomInfo: %w", err) } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return nil } response.RoomExists = true @@ -390,7 +390,7 @@ func (r *Queryer) QueryMissingEvents( if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return fmt.Errorf("missing RoomInfo for room %s", events[0].RoomID()) } @@ -429,7 +429,7 @@ func (r *Queryer) QueryStateAndAuthChain( if err != nil { return err } - if info == nil || info.IsStub { + if info == nil || info.IsStub() { return nil } response.RoomExists = true @@ -774,7 +774,7 @@ func (r *Queryer) QueryRestrictedJoinAllowed(ctx context.Context, req *api.Query if err != nil { return fmt.Errorf("r.DB.RoomInfo: %w", err) } - if roomInfo == nil || roomInfo.IsStub { + if roomInfo == nil || roomInfo.IsStub() { return nil // fmt.Errorf("room %q doesn't exist or is stub room", req.RoomID) } // If the room version doesn't allow restricted joins then don't @@ -837,7 +837,7 @@ func (r *Queryer) QueryRestrictedJoinAllowed(ctx context.Context, req *api.Query // See if the room exists. If it doesn't exist or if it's a stub // room entry then we can't check memberships. targetRoomInfo, err := r.DB.RoomInfo(ctx, rule.RoomID) - if err != nil || targetRoomInfo == nil || targetRoomInfo.IsStub { + if err != nil || targetRoomInfo == nil || targetRoomInfo.IsStub() { res.Resident = false continue } diff --git a/roomserver/storage/postgres/rooms_table.go b/roomserver/storage/postgres/rooms_table.go index 24362af74..994399532 100644 --- a/roomserver/storage/postgres/rooms_table.go +++ b/roomserver/storage/postgres/rooms_table.go @@ -147,14 +147,16 @@ func (s *roomStatements) InsertRoomNID( func (s *roomStatements) SelectRoomInfo(ctx context.Context, txn *sql.Tx, roomID string) (*types.RoomInfo, error) { var info types.RoomInfo var latestNIDs pq.Int64Array + var stateSnapshotNID types.StateSnapshotNID stmt := sqlutil.TxStmt(txn, s.selectRoomInfoStmt) err := stmt.QueryRowContext(ctx, roomID).Scan( - &info.RoomVersion, &info.RoomNID, &info.StateSnapshotNID, &latestNIDs, + &info.RoomVersion, &info.RoomNID, &stateSnapshotNID, &latestNIDs, ) if err == sql.ErrNoRows { return nil, nil } - info.IsStub = len(latestNIDs) == 0 + info.SetStateSnapshotNID(stateSnapshotNID) + info.SetIsStub(len(latestNIDs) == 0) return &info, err } diff --git a/roomserver/storage/shared/room_updater.go b/roomserver/storage/shared/room_updater.go index c35ac653c..a7ae26d43 100644 --- a/roomserver/storage/shared/room_updater.go +++ b/roomserver/storage/shared/room_updater.go @@ -229,8 +229,8 @@ func (u *RoomUpdater) SetLatestEvents( // Since it's entirely possible that this types.RoomInfo came from the // cache, we should make sure to update that entry so that the next run // works from live data. - u.roomInfo.StateSnapshotNID = currentStateSnapshotNID - u.roomInfo.IsStub = false + u.roomInfo.SetStateSnapshotNID(currentStateSnapshotNID) + u.roomInfo.SetIsStub(false) return nil }) } diff --git a/roomserver/storage/shared/storage.go b/roomserver/storage/shared/storage.go index daac4e821..05b897149 100644 --- a/roomserver/storage/shared/storage.go +++ b/roomserver/storage/shared/storage.go @@ -1032,7 +1032,7 @@ func (d *Database) GetStateEvent(ctx context.Context, roomID, evType, stateKey s return nil, fmt.Errorf("room %s doesn't exist", roomID) } // e.g invited rooms - if roomInfo.IsStub { + if roomInfo.IsStub() { return nil, nil } eventTypeNID, err := d.EventTypesTable.SelectEventTypeNID(ctx, nil, evType) @@ -1051,7 +1051,7 @@ func (d *Database) GetStateEvent(ctx context.Context, roomID, evType, stateKey s if err != nil { return nil, err } - entries, err := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID) + entries, err := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID()) if err != nil { return nil, err } @@ -1097,7 +1097,7 @@ func (d *Database) GetStateEventsWithEventType(ctx context.Context, roomID, evTy return nil, fmt.Errorf("room %s doesn't exist", roomID) } // e.g invited rooms - if roomInfo.IsStub { + if roomInfo.IsStub() { return nil, nil } eventTypeNID, err := d.EventTypesTable.SelectEventTypeNID(ctx, nil, evType) @@ -1108,7 +1108,7 @@ func (d *Database) GetStateEventsWithEventType(ctx context.Context, roomID, evTy if err != nil { return nil, err } - entries, err := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID) + entries, err := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID()) if err != nil { return nil, err } @@ -1225,10 +1225,10 @@ func (d *Database) GetBulkStateContent(ctx context.Context, roomIDs []string, tu return nil, fmt.Errorf("GetBulkStateContent: failed to load room info for room %s : %w", roomID, err2) } // for unknown rooms or rooms which we don't have the current state, skip them. - if roomInfo == nil || roomInfo.IsStub { + if roomInfo == nil || roomInfo.IsStub() { continue } - entries, err2 := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID) + entries, err2 := d.loadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID()) if err2 != nil { return nil, fmt.Errorf("GetBulkStateContent: failed to load state for room %s : %w", roomID, err2) } diff --git a/roomserver/storage/sqlite3/rooms_table.go b/roomserver/storage/sqlite3/rooms_table.go index 03ad4b3d0..25b611b3e 100644 --- a/roomserver/storage/sqlite3/rooms_table.go +++ b/roomserver/storage/sqlite3/rooms_table.go @@ -129,9 +129,10 @@ func (s *roomStatements) SelectRoomIDsWithEvents(ctx context.Context, txn *sql.T func (s *roomStatements) SelectRoomInfo(ctx context.Context, txn *sql.Tx, roomID string) (*types.RoomInfo, error) { var info types.RoomInfo var latestNIDsJSON string + var stateSnapshotNID types.StateSnapshotNID stmt := sqlutil.TxStmt(txn, s.selectRoomInfoStmt) err := stmt.QueryRowContext(ctx, roomID).Scan( - &info.RoomVersion, &info.RoomNID, &info.StateSnapshotNID, &latestNIDsJSON, + &info.RoomVersion, &info.RoomNID, &stateSnapshotNID, &latestNIDsJSON, ) if err != nil { if err == sql.ErrNoRows { @@ -143,7 +144,8 @@ func (s *roomStatements) SelectRoomInfo(ctx context.Context, txn *sql.Tx, roomID if err = json.Unmarshal([]byte(latestNIDsJSON), &latestNIDs); err != nil { return nil, err } - info.IsStub = len(latestNIDs) == 0 + info.SetStateSnapshotNID(stateSnapshotNID) + info.SetIsStub(len(latestNIDs) == 0) return &info, err } diff --git a/roomserver/storage/tables/rooms_table_test.go b/roomserver/storage/tables/rooms_table_test.go index 0a02369a1..eddd012c8 100644 --- a/roomserver/storage/tables/rooms_table_test.go +++ b/roomserver/storage/tables/rooms_table_test.go @@ -63,12 +63,12 @@ func TestRoomsTable(t *testing.T) { roomInfo, err := tab.SelectRoomInfo(ctx, nil, room.ID) assert.NoError(t, err) - assert.Equal(t, &types.RoomInfo{ - RoomNID: wantRoomNID, - RoomVersion: room.Version, - StateSnapshotNID: 0, - IsStub: true, // there are no latestEventNIDs - }, roomInfo) + expected := &types.RoomInfo{ + RoomNID: wantRoomNID, + RoomVersion: room.Version, + } + expected.SetIsStub(true) // there are no latestEventNIDs + assert.Equal(t, expected, roomInfo) roomInfo, err = tab.SelectRoomInfo(ctx, nil, "!doesnotexist:localhost") assert.NoError(t, err) @@ -103,12 +103,12 @@ func TestRoomsTable(t *testing.T) { roomInfo, err = tab.SelectRoomInfo(ctx, nil, room.ID) assert.NoError(t, err) - assert.Equal(t, &types.RoomInfo{ - RoomNID: wantRoomNID, - RoomVersion: room.Version, - StateSnapshotNID: 1, - IsStub: false, - }, roomInfo) + expected = &types.RoomInfo{ + RoomNID: wantRoomNID, + RoomVersion: room.Version, + } + expected.SetStateSnapshotNID(1) + assert.Equal(t, expected, roomInfo) eventNIDs, snapshotNID, err := tab.SelectLatestEventNIDs(ctx, nil, wantRoomNID) assert.NoError(t, err) diff --git a/roomserver/types/types.go b/roomserver/types/types.go index bc01ca33c..726659ea0 100644 --- a/roomserver/types/types.go +++ b/roomserver/types/types.go @@ -19,6 +19,7 @@ import ( "encoding/json" "sort" "strings" + "sync" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" @@ -279,8 +280,33 @@ func (e RejectedError) Error() string { return string(e) } // RoomInfo contains metadata about a room type RoomInfo struct { + mu sync.RWMutex RoomNID RoomNID RoomVersion gomatrixserverlib.RoomVersion - StateSnapshotNID StateSnapshotNID - IsStub bool + stateSnapshotNID StateSnapshotNID + isStub bool +} + +func (r *RoomInfo) StateSnapshotNID() StateSnapshotNID { + r.mu.RLock() + defer r.mu.RUnlock() + return r.stateSnapshotNID +} + +func (r *RoomInfo) IsStub() bool { + r.mu.RLock() + defer r.mu.RUnlock() + return r.isStub +} + +func (r *RoomInfo) SetStateSnapshotNID(nid StateSnapshotNID) { + r.mu.Lock() + defer r.mu.Unlock() + r.stateSnapshotNID = nid +} + +func (r *RoomInfo) SetIsStub(isStub bool) { + r.mu.Lock() + defer r.mu.Unlock() + r.isStub = isStub }