From b4bd0cc0f5440fe6b61de050c49ed723b26d7bb4 Mon Sep 17 00:00:00 2001 From: Kegsay Date: Tue, 8 Sep 2020 17:30:05 +0100 Subject: [PATCH 01/11] Track goids when running with tracing enabled (#1413) * Track goids when running with tracing enabled * Linting --- internal/sqlutil/trace.go | 32 ++++++++++++++++++++++++++++ internal/sqlutil/writer_exclusive.go | 6 ++++++ 2 files changed, 38 insertions(+) diff --git a/internal/sqlutil/trace.go b/internal/sqlutil/trace.go index fbd983bec..248dbe38b 100644 --- a/internal/sqlutil/trace.go +++ b/internal/sqlutil/trace.go @@ -22,7 +22,10 @@ import ( "io" "os" "regexp" + "runtime" + "strconv" "strings" + "sync" "time" "github.com/matrix-org/dendrite/internal/config" @@ -31,6 +34,7 @@ import ( ) var tracingEnabled = os.Getenv("DENDRITE_TRACE_SQL") == "1" +var goidToWriter sync.Map type traceInterceptor struct { sqlmw.NullInterceptor @@ -40,6 +44,8 @@ func (in *traceInterceptor) StmtQueryContext(ctx context.Context, stmt driver.St startedAt := time.Now() rows, err := stmt.QueryContext(ctx, args) + trackGoID(query) + logrus.WithField("duration", time.Since(startedAt)).WithField(logrus.ErrorKey, err).Debug("executed sql query ", query, " args: ", args) return rows, err @@ -49,6 +55,8 @@ func (in *traceInterceptor) StmtExecContext(ctx context.Context, stmt driver.Stm startedAt := time.Now() result, err := stmt.ExecContext(ctx, args) + trackGoID(query) + logrus.WithField("duration", time.Since(startedAt)).WithField(logrus.ErrorKey, err).Debug("executed sql query ", query, " args: ", args) return result, err @@ -75,6 +83,19 @@ func (in *traceInterceptor) RowsNext(c context.Context, rows driver.Rows, dest [ return err } +func trackGoID(query string) { + thisGoID := goid() + if _, ok := goidToWriter.Load(thisGoID); ok { + return // we're on a writer goroutine + } + + q := strings.TrimSpace(query) + if strings.HasPrefix(q, "SELECT") { + return // SELECTs can go on other goroutines + } + logrus.Warnf("unsafe goid: SQL executed not on an ExclusiveWriter: %s", q) +} + // Open opens a database specified by its database driver name and a driver-specific data source name, // usually consisting of at least a database name and connection information. Includes tracing driver // if DENDRITE_TRACE_SQL=1 @@ -119,3 +140,14 @@ func Open(dbProperties *config.DatabaseOptions) (*sql.DB, error) { func init() { registerDrivers() } + +func goid() int { + var buf [64]byte + n := runtime.Stack(buf[:], false) + idField := strings.Fields(strings.TrimPrefix(string(buf[:n]), "goroutine "))[0] + id, err := strconv.Atoi(idField) + if err != nil { + panic(fmt.Sprintf("cannot get goroutine id: %v", err)) + } + return id +} diff --git a/internal/sqlutil/writer_exclusive.go b/internal/sqlutil/writer_exclusive.go index 002bc32cf..91dd77e4d 100644 --- a/internal/sqlutil/writer_exclusive.go +++ b/internal/sqlutil/writer_exclusive.go @@ -60,6 +60,12 @@ func (w *ExclusiveWriter) run() { if !w.running.CAS(false, true) { return } + if tracingEnabled { + gid := goid() + goidToWriter.Store(gid, w) + defer goidToWriter.Delete(gid) + } + defer w.running.Store(false) for task := range w.todo { if task.db != nil && task.txn != nil { From a0f2a4510fd6f838e12e1654148a17b653574f8b Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 8 Sep 2020 17:47:54 +0100 Subject: [PATCH 02/11] Exclude deleted keys from selectBatchDeviceKeysSQL (#1412) --- keyserver/storage/postgres/device_keys_table.go | 2 +- keyserver/storage/sqlite3/device_keys_table.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/keyserver/storage/postgres/device_keys_table.go b/keyserver/storage/postgres/device_keys_table.go index 779d02c03..e5bec8f6f 100644 --- a/keyserver/storage/postgres/device_keys_table.go +++ b/keyserver/storage/postgres/device_keys_table.go @@ -53,7 +53,7 @@ const selectDeviceKeysSQL = "" + "SELECT key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1 AND device_id=$2" const selectBatchDeviceKeysSQL = "" + - "SELECT device_id, key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1" + "SELECT device_id, key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1 AND key_json <> ''" const selectMaxStreamForUserSQL = "" + "SELECT MAX(stream_id) FROM keyserver_device_keys WHERE user_id=$1" diff --git a/keyserver/storage/sqlite3/device_keys_table.go b/keyserver/storage/sqlite3/device_keys_table.go index 195429f08..e7ff9976d 100644 --- a/keyserver/storage/sqlite3/device_keys_table.go +++ b/keyserver/storage/sqlite3/device_keys_table.go @@ -50,7 +50,7 @@ const selectDeviceKeysSQL = "" + "SELECT key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1 AND device_id=$2" const selectBatchDeviceKeysSQL = "" + - "SELECT device_id, key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1" + "SELECT device_id, key_json, stream_id, display_name FROM keyserver_device_keys WHERE user_id=$1 AND key_json <> ''" const selectMaxStreamForUserSQL = "" + "SELECT MAX(stream_id) FROM keyserver_device_keys WHERE user_id=$1" From 35564dd73c48b16b97cd1a972a9b9bc65ec6d7ef Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 8 Sep 2020 17:48:07 +0100 Subject: [PATCH 03/11] Process membership updates in writers (#1414) --- .../storage/shared/membership_updater.go | 148 ++++++++++-------- 1 file changed, 81 insertions(+), 67 deletions(-) diff --git a/roomserver/storage/shared/membership_updater.go b/roomserver/storage/shared/membership_updater.go index 329813bfc..834af6069 100644 --- a/roomserver/storage/shared/membership_updater.go +++ b/roomserver/storage/shared/membership_updater.go @@ -79,89 +79,103 @@ func (u *MembershipUpdater) IsLeave() bool { // SetToInvite implements types.MembershipUpdater func (u *MembershipUpdater) SetToInvite(event gomatrixserverlib.Event) (bool, error) { - senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, event.Sender()) - if err != nil { - return false, fmt.Errorf("u.d.AssignStateKeyNID: %w", err) - } - inserted, err := u.d.InvitesTable.InsertInviteEvent( - u.ctx, u.txn, event.EventID(), u.roomNID, u.targetUserNID, senderUserNID, event.JSON(), - ) - if err != nil { - return false, fmt.Errorf("u.d.InvitesTable.InsertInviteEvent: %w", err) - } - if u.membership != tables.MembershipStateInvite { - if err = u.d.MembershipTable.UpdateMembership( - u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, tables.MembershipStateInvite, 0, - ); err != nil { - return false, fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + var inserted bool + err := u.d.Writer.Do(u.d.DB, u.txn, func(txn *sql.Tx) error { + senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, event.Sender()) + if err != nil { + return fmt.Errorf("u.d.AssignStateKeyNID: %w", err) } - } - return inserted, nil + inserted, err = u.d.InvitesTable.InsertInviteEvent( + u.ctx, u.txn, event.EventID(), u.roomNID, u.targetUserNID, senderUserNID, event.JSON(), + ) + if err != nil { + return fmt.Errorf("u.d.InvitesTable.InsertInviteEvent: %w", err) + } + if u.membership != tables.MembershipStateInvite { + if err = u.d.MembershipTable.UpdateMembership( + u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, tables.MembershipStateInvite, 0, + ); err != nil { + return fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + } + } + return nil + }) + return inserted, err } // SetToJoin implements types.MembershipUpdater func (u *MembershipUpdater) SetToJoin(senderUserID string, eventID string, isUpdate bool) ([]string, error) { var inviteEventIDs []string - senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, senderUserID) - if err != nil { - return nil, fmt.Errorf("u.d.AssignStateKeyNID: %w", err) - } - - // If this is a join event update, there is no invite to update - if !isUpdate { - inviteEventIDs, err = u.d.InvitesTable.UpdateInviteRetired( - u.ctx, u.txn, u.roomNID, u.targetUserNID, - ) + err := u.d.Writer.Do(u.d.DB, u.txn, func(txn *sql.Tx) error { + senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, senderUserID) if err != nil { - return nil, fmt.Errorf("u.d.InvitesTables.UpdateInviteRetired: %w", err) + return fmt.Errorf("u.d.AssignStateKeyNID: %w", err) } - } - // Look up the NID of the new join event - nIDs, err := u.d.EventNIDs(u.ctx, []string{eventID}) - if err != nil { - return nil, fmt.Errorf("u.d.EventNIDs: %w", err) - } - - if u.membership != tables.MembershipStateJoin || isUpdate { - if err = u.d.MembershipTable.UpdateMembership( - u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, - tables.MembershipStateJoin, nIDs[eventID], - ); err != nil { - return nil, fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + // If this is a join event update, there is no invite to update + if !isUpdate { + inviteEventIDs, err = u.d.InvitesTable.UpdateInviteRetired( + u.ctx, u.txn, u.roomNID, u.targetUserNID, + ) + if err != nil { + return fmt.Errorf("u.d.InvitesTables.UpdateInviteRetired: %w", err) + } } - } - return inviteEventIDs, nil + // Look up the NID of the new join event + nIDs, err := u.d.EventNIDs(u.ctx, []string{eventID}) + if err != nil { + return fmt.Errorf("u.d.EventNIDs: %w", err) + } + + if u.membership != tables.MembershipStateJoin || isUpdate { + if err = u.d.MembershipTable.UpdateMembership( + u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, + tables.MembershipStateJoin, nIDs[eventID], + ); err != nil { + return fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + } + } + + return nil + }) + + return inviteEventIDs, err } // SetToLeave implements types.MembershipUpdater func (u *MembershipUpdater) SetToLeave(senderUserID string, eventID string) ([]string, error) { - senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, senderUserID) - if err != nil { - return nil, fmt.Errorf("u.d.AssignStateKeyNID: %w", err) - } - inviteEventIDs, err := u.d.InvitesTable.UpdateInviteRetired( - u.ctx, u.txn, u.roomNID, u.targetUserNID, - ) - if err != nil { - return nil, fmt.Errorf("u.d.InvitesTable.updateInviteRetired: %w", err) - } + var inviteEventIDs []string - // Look up the NID of the new leave event - nIDs, err := u.d.EventNIDs(u.ctx, []string{eventID}) - if err != nil { - return nil, fmt.Errorf("u.d.EventNIDs: %w", err) - } - - if u.membership != tables.MembershipStateLeaveOrBan { - if err = u.d.MembershipTable.UpdateMembership( - u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, - tables.MembershipStateLeaveOrBan, nIDs[eventID], - ); err != nil { - return nil, fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + err := u.d.Writer.Do(u.d.DB, u.txn, func(txn *sql.Tx) error { + senderUserNID, err := u.d.assignStateKeyNID(u.ctx, u.txn, senderUserID) + if err != nil { + return fmt.Errorf("u.d.AssignStateKeyNID: %w", err) } - } - return inviteEventIDs, nil + inviteEventIDs, err = u.d.InvitesTable.UpdateInviteRetired( + u.ctx, u.txn, u.roomNID, u.targetUserNID, + ) + if err != nil { + return fmt.Errorf("u.d.InvitesTable.updateInviteRetired: %w", err) + } + + // Look up the NID of the new leave event + nIDs, err := u.d.EventNIDs(u.ctx, []string{eventID}) + if err != nil { + return fmt.Errorf("u.d.EventNIDs: %w", err) + } + + if u.membership != tables.MembershipStateLeaveOrBan { + if err = u.d.MembershipTable.UpdateMembership( + u.ctx, u.txn, u.roomNID, u.targetUserNID, senderUserNID, + tables.MembershipStateLeaveOrBan, nIDs[eventID], + ); err != nil { + return fmt.Errorf("u.d.MembershipTable.UpdateMembership: %w", err) + } + } + + return nil + }) + return inviteEventIDs, err } From de53608f98cde121b90108267c7f369917a00ebd Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 9 Sep 2020 02:14:06 +0100 Subject: [PATCH 04/11] fix nightmare bug where sqlite doesn't let you use out of order sub strings --- syncapi/storage/sqlite3/peeks_table.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/syncapi/storage/sqlite3/peeks_table.go b/syncapi/storage/sqlite3/peeks_table.go index e1e2515ad..add1b4ff3 100644 --- a/syncapi/storage/sqlite3/peeks_table.go +++ b/syncapi/storage/sqlite3/peeks_table.go @@ -53,8 +53,9 @@ const deletePeeksSQL = "" + // we care about all the peeks which were created in this range, deleted in this range, // or were created before this range but haven't been deleted yet. +// BEWARE: sqlite chokes on out of order substitution strings. const selectPeeksInRangeSQL = "" + - "SELECT room_id, deleted, (id > $3 AND id <= $4) AS changed FROM syncapi_peeks WHERE user_id = $1 AND device_id = $2 AND ((id <= $3 AND NOT deleted) OR (id > $3 AND id <= $4))" + "SELECT id, room_id, deleted FROM syncapi_peeks WHERE user_id = $1 AND device_id = $2 AND ((id <= $3 AND NOT deleted=true) OR (id > $3 AND id <= $4))" const selectPeekingDevicesSQL = "" + "SELECT room_id, user_id, device_id FROM syncapi_peeks WHERE deleted=false" @@ -148,11 +149,11 @@ func (s *peekStatements) SelectPeeksInRange( for rows.Next() { peek := types.Peek{} - var changed bool - if err = rows.Scan(&peek.RoomID, &peek.Deleted, &changed); err != nil { + var id types.StreamPosition + if err = rows.Scan(&id, &peek.RoomID, &peek.Deleted); err != nil { return } - peek.New = changed && !peek.Deleted + peek.New = (id > r.Low() && id <= r.High()) && !peek.Deleted peeks = append(peeks, peek) } From b45436aab0d315d5e7436b262a3ef40e40bfc134 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 9 Sep 2020 02:15:58 +0100 Subject: [PATCH 05/11] handle exclusive writer txn for cleanliness --- syncapi/storage/shared/syncserver.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/syncapi/storage/shared/syncserver.go b/syncapi/storage/shared/syncserver.go index 5395b9c5d..76db40537 100644 --- a/syncapi/storage/shared/syncserver.go +++ b/syncapi/storage/shared/syncserver.go @@ -30,7 +30,7 @@ import ( "github.com/matrix-org/dendrite/syncapi/storage/tables" "github.com/matrix-org/dendrite/syncapi/types" "github.com/matrix-org/gomatrixserverlib" - "github.com/sirupsen/logrus" + log "github.com/sirupsen/logrus" ) // Database is a temporary struct until we have made syncserver.go the same for both pq/sqlite @@ -169,8 +169,8 @@ func (d *Database) RetireInviteEvent( func (d *Database) AddPeek( ctx context.Context, roomID, userID, deviceID string, ) (sp types.StreamPosition, err error) { - _ = d.Writer.Do(nil, nil, func(_ *sql.Tx) error { - sp, err = d.Peeks.InsertPeek(ctx, nil, roomID, userID, deviceID) + _ = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { + sp, err = d.Peeks.InsertPeek(ctx, txn, roomID, userID, deviceID) return nil }) return @@ -182,8 +182,8 @@ func (d *Database) AddPeek( func (d *Database) DeletePeeks( ctx context.Context, roomID, userID string, ) (sp types.StreamPosition, err error) { - _ = d.Writer.Do(nil, nil, func(_ *sql.Tx) error { - sp, err = d.Peeks.DeletePeeks(ctx, nil, roomID, userID) + _ = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { + sp, err = d.Peeks.DeletePeeks(ctx, txn, roomID, userID) return nil }) if err == sql.ErrNoRows { @@ -230,7 +230,7 @@ func (d *Database) StreamEventsToEvents(device *userapi.Device, in []types.Strea "transaction_id", in[i].TransactionID.TransactionID, ) if err != nil { - logrus.WithFields(logrus.Fields{ + log.WithFields(log.Fields{ "event_id": out[i].EventID(), }).WithError(err).Warnf("Failed to add transaction ID to event") } @@ -624,7 +624,7 @@ func (d *Database) RedactEvent(ctx context.Context, redactedEventID string, reda return err } if len(redactedEvents) == 0 { - logrus.WithField("event_id", redactedEventID).WithField("redaction_event", redactedBecause.EventID()).Warnf("missing redacted event for redaction") + log.WithField("event_id", redactedEventID).WithField("redaction_event", redactedBecause.EventID()).Warnf("missing redacted event for redaction") return nil } eventToRedact := redactedEvents[0].Unwrap() From 052351e0bfe9cc99bcaed591533281d508dea814 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 9 Sep 2020 02:21:28 +0100 Subject: [PATCH 06/11] go fmt --- syncapi/storage/postgres/peeks_table.go | 2 +- syncapi/storage/postgres/syncserver.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/syncapi/storage/postgres/peeks_table.go b/syncapi/storage/postgres/peeks_table.go index d0d8bcf6b..23981eeab 100644 --- a/syncapi/storage/postgres/peeks_table.go +++ b/syncapi/storage/postgres/peeks_table.go @@ -78,7 +78,7 @@ func NewPostgresPeeksTable(db *sql.DB) (tables.Peeks, error) { return nil, err } s := &peekStatements{ - db: db, + db: db, } if s.insertPeekStmt, err = db.Prepare(insertPeekSQL); err != nil { return nil, err diff --git a/syncapi/storage/postgres/syncserver.go b/syncapi/storage/postgres/syncserver.go index ad929a402..7f19722ae 100644 --- a/syncapi/storage/postgres/syncserver.go +++ b/syncapi/storage/postgres/syncserver.go @@ -86,7 +86,7 @@ func NewDatabase(dbProperties *config.DatabaseOptions) (*SyncServerDatasource, e DB: d.db, Writer: d.writer, Invites: invites, - Peeks: peeks, + Peeks: peeks, AccountData: accountData, OutputEvents: events, Topology: topology, From 230054991cffa15025ce7f268338770d6c29f24b Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 9 Sep 2020 02:37:17 +0100 Subject: [PATCH 07/11] fix lint --- syncapi/sync/notifier.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/syncapi/sync/notifier.go b/syncapi/sync/notifier.go index e7642c3d8..fcac3f16c 100644 --- a/syncapi/sync/notifier.go +++ b/syncapi/sync/notifier.go @@ -339,6 +339,7 @@ func (n *Notifier) addPeekingDevice(roomID, userID, deviceID string) { } // Not thread-safe: must be called on the OnNewEvent goroutine only +// nolint:unused func (n *Notifier) removePeekingDevice(roomID, userID, deviceID string) { if _, ok := n.roomIDToPeekingDevices[roomID]; !ok { n.roomIDToPeekingDevices[roomID] = make(peekingDeviceSet) @@ -409,6 +410,7 @@ func (s peekingDeviceSet) add(d types.PeekingDevice) { s[d] = true } +// nolint:unused func (s peekingDeviceSet) remove(d types.PeekingDevice) { delete(s, d) } From f09afe0f65687f0c99ea5822231fb6efb6bf1a20 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 9 Sep 2020 10:11:38 +0100 Subject: [PATCH 08/11] Remove accidental formerly-untracked file --- syncapi/sync/provider.go | 11 ----------- 1 file changed, 11 deletions(-) delete mode 100644 syncapi/sync/provider.go diff --git a/syncapi/sync/provider.go b/syncapi/sync/provider.go deleted file mode 100644 index fd8bfe03d..000000000 --- a/syncapi/sync/provider.go +++ /dev/null @@ -1,11 +0,0 @@ -package sync - -import "github.com/matrix-org/dendrite/syncapi/types" - -type SyncProvider interface { - WaitFor() -} - -type SyncStream interface { - GetLatestPosition() types.StreamPosition -} From 15349d8287fa3880828c576fad66a2e556cea751 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 9 Sep 2020 16:12:46 +0100 Subject: [PATCH 09/11] Return errors from SQL statements to handle rollbacks correctly --- syncapi/storage/shared/syncserver.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/syncapi/storage/shared/syncserver.go b/syncapi/storage/shared/syncserver.go index 76db40537..38362cc35 100644 --- a/syncapi/storage/shared/syncserver.go +++ b/syncapi/storage/shared/syncserver.go @@ -169,9 +169,9 @@ func (d *Database) RetireInviteEvent( func (d *Database) AddPeek( ctx context.Context, roomID, userID, deviceID string, ) (sp types.StreamPosition, err error) { - _ = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { + err = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { sp, err = d.Peeks.InsertPeek(ctx, txn, roomID, userID, deviceID) - return nil + return err }) return } @@ -182,9 +182,9 @@ func (d *Database) AddPeek( func (d *Database) DeletePeeks( ctx context.Context, roomID, userID string, ) (sp types.StreamPosition, err error) { - _ = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { + err = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { sp, err = d.Peeks.DeletePeeks(ctx, txn, roomID, userID) - return nil + return err }) if err == sql.ErrNoRows { err = nil From 34b89619f268143589c230252316057ea15363bd Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 9 Sep 2020 16:53:21 +0100 Subject: [PATCH 10/11] Fix sqlite DeletePeeks API to match postgres; fix bug which incorrectly altered sp when nothing is deleted --- syncapi/consumers/roomserver.go | 10 ++++++---- syncapi/storage/shared/syncserver.go | 1 + syncapi/storage/sqlite3/peeks_table.go | 20 +++++++++++++++----- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/syncapi/consumers/roomserver.go b/syncapi/consumers/roomserver.go index 404f28b9c..b6ab9bd50 100644 --- a/syncapi/consumers/roomserver.go +++ b/syncapi/consumers/roomserver.go @@ -216,10 +216,12 @@ func (s *OutputRoomEventConsumer) notifyJoinedPeeks(ctx context.Context, ev *gom } // cancel any peeks for it - sp, err = s.db.DeletePeeks(ctx, ev.RoomID(), *ev.StateKey()) - // XXX: should we do anything with this new streampos? - if err != nil { - return sp, fmt.Errorf("s.db.DeletePeeks: %w", err) + peekSP, peekErr := s.db.DeletePeeks(ctx, ev.RoomID(), *ev.StateKey()) + if peekErr != nil { + return sp, fmt.Errorf("s.db.DeletePeeks: %w", peekErr) + } + if peekSP > 0 { + sp = peekSP } } return sp, nil diff --git a/syncapi/storage/shared/syncserver.go b/syncapi/storage/shared/syncserver.go index 38362cc35..bb91309cb 100644 --- a/syncapi/storage/shared/syncserver.go +++ b/syncapi/storage/shared/syncserver.go @@ -187,6 +187,7 @@ func (d *Database) DeletePeeks( return err }) if err == sql.ErrNoRows { + sp = 0 err = nil } return diff --git a/syncapi/storage/sqlite3/peeks_table.go b/syncapi/storage/sqlite3/peeks_table.go index add1b4ff3..409b03019 100644 --- a/syncapi/storage/sqlite3/peeks_table.go +++ b/syncapi/storage/sqlite3/peeks_table.go @@ -129,13 +129,23 @@ func (s *peekStatements) DeletePeek( func (s *peekStatements) DeletePeeks( ctx context.Context, txn *sql.Tx, roomID, userID string, -) (streamPos types.StreamPosition, err error) { - streamPos, err = s.streamIDStatements.nextStreamID(ctx, txn) +) (types.StreamPosition, error) { + streamPos, err := s.streamIDStatements.nextStreamID(ctx, txn) if err != nil { - return + return 0, err } - _, err = sqlutil.TxStmt(txn, s.deletePeeksStmt).ExecContext(ctx, streamPos, roomID, userID) - return + result, err := sqlutil.TxStmt(txn, s.deletePeeksStmt).ExecContext(ctx, streamPos, roomID, userID) + if err != nil { + return 0, err + } + numAffected, err := result.RowsAffected() + if err != nil { + return 0, err + } + if numAffected == 0 { + return 0, sql.ErrNoRows + } + return streamPos, nil } func (s *peekStatements) SelectPeeksInRange( From af472377b7e95260e4a476a42ca293d08a884b10 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 9 Sep 2020 17:17:44 +0100 Subject: [PATCH 11/11] Add peeking tests to whitelist --- sytest-whitelist | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sytest-whitelist b/sytest-whitelist index 93d2de593..0adeaee6f 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -465,3 +465,9 @@ After changing password, can log in with new password After changing password, existing session still works After changing password, different sessions can optionally be kept After changing password, a different session no longer works by default +Local users can peek into world_readable rooms by room ID +We can't peek into rooms with shared history_visibility +We can't peek into rooms with invited history_visibility +We can't peek into rooms with joined history_visibility +Local users can peek by room alias +Peeked rooms only turn up in the sync for the device who peeked them \ No newline at end of file