From 2250768be16bd0e6b3a6a72b5e55eb3e2ad6e3c6 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 3 Aug 2022 17:14:21 +0100 Subject: [PATCH] Remove roominfo cache (#2615) * Remove roominfo cache It's the source of a number of race conditions which are seemingly causing bugs and CI failures. * Make the linter less sad --- internal/caching/cache_roominfo.go | 33 ---------------------- internal/caching/cache_roomservernids.go | 1 - internal/caching/caches.go | 1 - internal/caching/impl_ristretto.go | 7 ----- roomserver/storage/shared/storage.go | 35 ++++++------------------ 5 files changed, 9 insertions(+), 68 deletions(-) delete mode 100644 internal/caching/cache_roominfo.go diff --git a/internal/caching/cache_roominfo.go b/internal/caching/cache_roominfo.go deleted file mode 100644 index 5dfed3c85..000000000 --- a/internal/caching/cache_roominfo.go +++ /dev/null @@ -1,33 +0,0 @@ -package caching - -import ( - "github.com/matrix-org/dendrite/roomserver/types" -) - -// WARNING: This cache is mutable because it's entirely possible that -// the IsStub or StateSnaphotNID fields can change, even though the -// room version and room NID fields will not. This is only safe because -// the RoomInfoCache is used ONLY within the roomserver and because it -// will be kept up-to-date by the latest events updater. It MUST NOT be -// used from other components as we currently have no way to invalidate -// the cache in downstream components. - -// RoomInfosCache contains the subset of functions needed for -// a room Info cache. It must only be used from the roomserver only -// It is not safe for use from other components. -type RoomInfoCache interface { - GetRoomInfo(roomID string) (roomInfo *types.RoomInfo, ok bool) - StoreRoomInfo(roomID string, roomInfo *types.RoomInfo) -} - -// GetRoomInfo must only be called from the roomserver only. It is not -// safe for use from other components. -func (c Caches) GetRoomInfo(roomID string) (*types.RoomInfo, bool) { - return c.RoomInfos.Get(roomID) -} - -// StoreRoomInfo must only be called from the roomserver only. It is not -// safe for use from other components. -func (c Caches) StoreRoomInfo(roomID string, roomInfo *types.RoomInfo) { - c.RoomInfos.Set(roomID, roomInfo) -} diff --git a/internal/caching/cache_roomservernids.go b/internal/caching/cache_roomservernids.go index f27154f19..88a5b28bc 100644 --- a/internal/caching/cache_roomservernids.go +++ b/internal/caching/cache_roomservernids.go @@ -7,7 +7,6 @@ import ( type RoomServerCaches interface { RoomServerNIDsCache RoomVersionCache - RoomInfoCache RoomServerEventsCache EventStateKeyCache } diff --git a/internal/caching/caches.go b/internal/caching/caches.go index f13f743d3..78c9ab7ee 100644 --- a/internal/caching/caches.go +++ b/internal/caching/caches.go @@ -29,7 +29,6 @@ type Caches struct { RoomServerRoomIDs Cache[types.RoomNID, string] // room NID -> room ID RoomServerEvents Cache[int64, *gomatrixserverlib.Event] // event NID -> event RoomServerStateKeys Cache[types.EventStateKeyNID, string] // event NID -> event state key - RoomInfos Cache[string, *types.RoomInfo] // room ID -> room info FederationPDUs Cache[int64, *gomatrixserverlib.HeaderedEvent] // queue NID -> PDU FederationEDUs Cache[int64, *gomatrixserverlib.EDU] // queue NID -> EDU SpaceSummaryRooms Cache[string, gomatrixserverlib.MSC2946SpacesResponse] // room ID -> space response diff --git a/internal/caching/impl_ristretto.go b/internal/caching/impl_ristretto.go index fdbbb4d72..fc0c8cc0f 100644 --- a/internal/caching/impl_ristretto.go +++ b/internal/caching/impl_ristretto.go @@ -35,7 +35,6 @@ const ( roomNIDsCache roomIDsCache roomEventsCache - roomInfosCache federationPDUsCache federationEDUsCache spaceSummaryRoomsCache @@ -106,12 +105,6 @@ func NewRistrettoCache(maxCost config.DataUnit, maxAge time.Duration, enableProm Prefix: eventStateKeyCache, MaxAge: maxAge, }, - RoomInfos: &RistrettoCachePartition[string, *types.RoomInfo]{ // room ID -> room info - cache: cache, - Prefix: roomInfosCache, - Mutable: true, - MaxAge: maxAge, - }, FederationPDUs: &RistrettoCostedCachePartition[int64, *gomatrixserverlib.HeaderedEvent]{ // queue NID -> PDU &RistrettoCachePartition[int64, *gomatrixserverlib.HeaderedEvent]{ cache: cache, diff --git a/roomserver/storage/shared/storage.go b/roomserver/storage/shared/storage.go index 9e6a4142c..cbf9c8b20 100644 --- a/roomserver/storage/shared/storage.go +++ b/roomserver/storage/shared/storage.go @@ -156,30 +156,15 @@ func (d *Database) RoomInfo(ctx context.Context, roomID string) (*types.RoomInfo } func (d *Database) roomInfo(ctx context.Context, txn *sql.Tx, roomID string) (*types.RoomInfo, error) { - roomInfo, ok := d.Cache.GetRoomInfo(roomID) - if ok && roomInfo != nil && !roomInfo.IsStub() { - // The data that's in the cache is not stubby, so return it. - return roomInfo, nil - } - // At this point we either don't have an entry in the cache, or - // it is stubby, so let's check the roomserver_rooms table again. - roomInfoFromDB, err := d.RoomsTable.SelectRoomInfo(ctx, txn, roomID) + roomInfo, err := d.RoomsTable.SelectRoomInfo(ctx, txn, roomID) if err != nil { return nil, err } - // If we have a stubby cache entry already, update it and return - // the reference to the cache entry. if roomInfo != nil { - roomInfo.CopyFrom(roomInfoFromDB) - return roomInfo, nil + d.Cache.StoreRoomServerRoomID(roomInfo.RoomNID, roomID) + d.Cache.StoreRoomVersion(roomID, roomInfo.RoomVersion) } - // Otherwise, try to admit the data into the cache and return the - // new reference from the database. - if roomInfoFromDB != nil { - d.Cache.StoreRoomServerRoomID(roomInfoFromDB.RoomNID, roomID) - d.Cache.StoreRoomInfo(roomID, roomInfoFromDB) - } - return roomInfoFromDB, err + return roomInfo, err } func (d *Database) AddState( @@ -504,8 +489,8 @@ func (d *Database) events( fetchNIDList := make([]types.RoomNID, 0, len(uniqueRoomNIDs)) for n := range uniqueRoomNIDs { if roomID, ok := d.Cache.GetRoomServerRoomID(n); ok { - if roomInfo, ok := d.Cache.GetRoomInfo(roomID); ok { - roomVersions[n] = roomInfo.RoomVersion + if roomVersion, ok := d.Cache.GetRoomVersion(roomID); ok { + roomVersions[n] = roomVersion continue } } @@ -762,9 +747,6 @@ func (d *Database) MissingAuthPrevEvents( func (d *Database) assignRoomNID( ctx context.Context, roomID string, roomVersion gomatrixserverlib.RoomVersion, ) (types.RoomNID, error) { - if roomInfo, ok := d.Cache.GetRoomInfo(roomID); ok { - return roomInfo.RoomNID, nil - } // Check if we already have a numeric ID in the database. roomNID, err := d.RoomsTable.SelectRoomNID(ctx, nil, roomID) if err == sql.ErrNoRows { @@ -837,8 +819,9 @@ func extractRoomVersionFromCreateEvent(event *gomatrixserverlib.Event) ( // "servers should not apply or send redactions to clients until both the redaction event and original event have been seen, and are valid." // https://matrix.org/docs/spec/rooms/v3#authorization-rules-for-events // These cases are: -// - This is a redaction event, redact the event it references if we know about it. -// - This is a normal event which may have been previously redacted. +// - This is a redaction event, redact the event it references if we know about it. +// - This is a normal event which may have been previously redacted. +// // In the first case, check if we have the referenced event then apply the redaction, else store it // in the redactions table with validated=FALSE. In the second case, check if there is a redaction for it: // if there is then apply the redactions and set validated=TRUE.