From 16e486ad96be2c81405f22229dcf58600e08e96d Mon Sep 17 00:00:00 2001 From: Tobi Smethurst <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 23 Jun 2021 18:42:20 +0200 Subject: [PATCH] Timeline bugfix (#60) * fix a stack overflow in the timeline * go fmt --- internal/timeline/get.go | 5 ++++ internal/timeline/index.go | 41 ++++++++++++++++++++++++- internal/timeline/manager.go | 45 +++++++++++++++++++++------- internal/timeline/timeline.go | 10 +++++-- internal/visibility/statusvisible.go | 25 ++++++++-------- 5 files changed, 100 insertions(+), 26 deletions(-) diff --git a/internal/timeline/get.go b/internal/timeline/get.go index 867e0940b..f07d81d55 100644 --- a/internal/timeline/get.go +++ b/internal/timeline/get.go @@ -13,7 +13,12 @@ func (t *timeline) Get(amount int, maxID string, sinceID string, minID string) ( l := t.log.WithFields(logrus.Fields{ "func": "Get", "accountID": t.accountID, + "amount": amount, + "maxID": maxID, + "sinceID": sinceID, + "minID": minID, }) + l.Debug("entering get") var statuses []*apimodel.Status var err error diff --git a/internal/timeline/index.go b/internal/timeline/index.go index 8d0506a9e..261722b74 100644 --- a/internal/timeline/index.go +++ b/internal/timeline/index.go @@ -10,6 +10,45 @@ import ( ) func (t *timeline) IndexBefore(statusID string, include bool, amount int) error { + filtered := []*gtsmodel.Status{} + offsetStatus := statusID + + if include { + s := >smodel.Status{} + if err := t.db.GetByID(statusID, s); err != nil { + return fmt.Errorf("IndexBefore: error getting initial status with id %s: %s", statusID, err) + } + filtered = append(filtered, s) + } + +grabloop: + for len(filtered) < amount { + statuses, err := t.db.GetStatusesWhereFollowing(t.accountID, "", offsetStatus, "", amount, false) + if err != nil { + if _, ok := err.(db.ErrNoEntries); ok { + break grabloop // we just don't have enough statuses left in the db so index what we've got and then bail + } + return fmt.Errorf("IndexBefore: error getting statuses from db: %s", err) + } + + for _, s := range statuses { + timelineable, err := t.filter.StatusHometimelineable(s, t.account) + if err != nil { + continue + } + if timelineable { + filtered = append(filtered, s) + } + offsetStatus = s.ID + } + } + + for _, s := range filtered { + if _, err := t.IndexOne(s.CreatedAt, s.ID, s.BoostOfID); err != nil { + return fmt.Errorf("IndexBefore: error indexing status with id %s: %s", s.ID, err) + } + } + return nil } @@ -41,7 +80,7 @@ grabloop: for _, s := range filtered { if _, err := t.IndexOne(s.CreatedAt, s.ID, s.BoostOfID); err != nil { - return fmt.Errorf("IndexBehindAndIncluding: error indexing status with id %s: %s", s.ID, err) + return fmt.Errorf("IndexBehind: error indexing status with id %s: %s", s.ID, err) } } diff --git a/internal/timeline/manager.go b/internal/timeline/manager.go index 923fd010b..d50c9f783 100644 --- a/internal/timeline/manager.go +++ b/internal/timeline/manager.go @@ -106,7 +106,10 @@ func (m *manager) Ingest(status *gtsmodel.Status, timelineAccountID string) (boo "statusID": status.ID, }) - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return false, err + } l.Trace("ingesting status") return t.IndexOne(status.CreatedAt, status.ID, status.BoostOfID) @@ -119,7 +122,10 @@ func (m *manager) IngestAndPrepare(status *gtsmodel.Status, timelineAccountID st "statusID": status.ID, }) - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return false, err + } l.Trace("ingesting status") return t.IndexAndPrepareOne(status.CreatedAt, status.ID) @@ -132,7 +138,10 @@ func (m *manager) Remove(statusID string, timelineAccountID string) (int, error) "statusID": statusID, }) - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return 0, err + } l.Trace("removing status") return t.Remove(statusID) @@ -144,7 +153,10 @@ func (m *manager) HomeTimeline(timelineAccountID string, maxID string, sinceID s "timelineAccountID": timelineAccountID, }) - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return nil, err + } statuses, err := t.Get(limit, maxID, sinceID, minID) if err != nil { @@ -154,7 +166,10 @@ func (m *manager) HomeTimeline(timelineAccountID string, maxID string, sinceID s } func (m *manager) GetIndexedLength(timelineAccountID string) int { - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return 0 + } return t.PostIndexLength() } @@ -164,13 +179,19 @@ func (m *manager) GetDesiredIndexLength() int { } func (m *manager) GetOldestIndexedID(timelineAccountID string) (string, error) { - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return "", err + } return t.OldestIndexedPostID() } func (m *manager) PrepareXFromTop(timelineAccountID string, limit int) error { - t := m.getOrCreateTimeline(timelineAccountID) + t, err := m.getOrCreateTimeline(timelineAccountID) + if err != nil { + return err + } return t.PrepareFromTop(limit) } @@ -198,11 +219,15 @@ func (m *manager) WipeStatusFromAllTimelines(statusID string) error { return err } -func (m *manager) getOrCreateTimeline(timelineAccountID string) Timeline { +func (m *manager) getOrCreateTimeline(timelineAccountID string) (Timeline, error) { var t Timeline i, ok := m.accountTimelines.Load(timelineAccountID) if !ok { - t = NewTimeline(timelineAccountID, m.db, m.tc, m.log) + var err error + t, err = NewTimeline(timelineAccountID, m.db, m.tc, m.log) + if err != nil { + return nil, err + } m.accountTimelines.Store(timelineAccountID, t) } else { t, ok = i.(Timeline) @@ -211,5 +236,5 @@ func (m *manager) getOrCreateTimeline(timelineAccountID string) Timeline { } } - return t + return t, nil } diff --git a/internal/timeline/timeline.go b/internal/timeline/timeline.go index 5e274b572..d0fadb19e 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -125,16 +125,22 @@ type timeline struct { } // NewTimeline returns a new Timeline for the given account ID -func NewTimeline(accountID string, db db.DB, typeConverter typeutils.TypeConverter, log *logrus.Logger) Timeline { +func NewTimeline(accountID string, db db.DB, typeConverter typeutils.TypeConverter, log *logrus.Logger) (Timeline, error) { + timelineOwnerAccount := >smodel.Account{} + if err := db.GetByID(accountID, timelineOwnerAccount); err != nil { + return nil, err + } + return &timeline{ postIndex: &postIndex{}, preparedPosts: &preparedPosts{}, accountID: accountID, + account: timelineOwnerAccount, db: db, filter: visibility.NewFilter(db, log), tc: typeConverter, log: log, - } + }, nil } func (t *timeline) Reset() error { diff --git a/internal/visibility/statusvisible.go b/internal/visibility/statusvisible.go index caf5cfcfd..c022be359 100644 --- a/internal/visibility/statusvisible.go +++ b/internal/visibility/statusvisible.go @@ -12,9 +12,8 @@ import ( func (f *filter) StatusVisible(targetStatus *gtsmodel.Status, requestingAccount *gtsmodel.Account) (bool, error) { l := f.log.WithFields(logrus.Fields{ - "func": "StatusVisible", - "statusID": targetStatus.ID, - "requestingAccountID": requestingAccount.ID, + "func": "StatusVisible", + "statusID": targetStatus.ID, }) relevantAccounts, err := f.pullRelevantAccountsFromStatus(targetStatus) @@ -49,6 +48,16 @@ func (f *filter) StatusVisible(targetStatus *gtsmodel.Status, requestingAccount } } + // If requesting account is nil, that means whoever requested the status didn't auth, or their auth failed. + // In this case, we can still serve the status if it's public, otherwise we definitely shouldn't. + if requestingAccount == nil { + if targetStatus.Visibility == gtsmodel.VisibilityPublic { + return true, nil + } + l.Trace("requesting account is nil but the target status isn't public") + return false, nil + } + // if the requesting user doesn't exist (anymore) then the status also shouldn't be visible // note: we only do this for local users if requestingAccount.Domain == "" { @@ -68,16 +77,6 @@ func (f *filter) StatusVisible(targetStatus *gtsmodel.Status, requestingAccount } } - // If requesting account is nil, that means whoever requested the status didn't auth, or their auth failed. - // In this case, we can still serve the status if it's public, otherwise we definitely shouldn't. - if requestingAccount == nil { - if targetStatus.Visibility == gtsmodel.VisibilityPublic { - return true, nil - } - l.Trace("requesting account is nil but the target status isn't public") - return false, nil - } - // if requesting account is suspended then don't show the status -- although they probably shouldn't have gotten // this far (ie., been authed) in the first place: this is just for safety. if !requestingAccount.SuspendedAt.IsZero() {