From d01763ee140bc92ec2f487259894ebc22d13a79f Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 29 Mar 2020 20:51:14 +0100 Subject: [PATCH] Protect against NPEs in notifications list (#10879) Unfortunately there appears to be potential race with notifications being set before the associated issue has been committed. This PR adds protection in to the notifications list to log any failures and remove these notifications from the display. References #10815 - and prevents the panic but does not completely fix this. Signed-off-by: Andrew Thornton --- models/notification.go | 76 +++++++++++++++++++++++++++--------- routers/user/notification.go | 21 ++++++++-- 2 files changed, 75 insertions(+), 22 deletions(-) diff --git a/models/notification.go b/models/notification.go index 05b0e92c9..daf13faf8 100644 --- a/models/notification.go +++ b/models/notification.go @@ -481,9 +481,9 @@ func (nl NotificationList) getPendingRepoIDs() []int64 { } // LoadRepos loads repositories from database -func (nl NotificationList) LoadRepos() (RepositoryList, error) { +func (nl NotificationList) LoadRepos() (RepositoryList, []int, error) { if len(nl) == 0 { - return RepositoryList{}, nil + return RepositoryList{}, []int{}, nil } var repoIDs = nl.getPendingRepoIDs() @@ -498,7 +498,7 @@ func (nl NotificationList) LoadRepos() (RepositoryList, error) { In("id", repoIDs[:limit]). Rows(new(Repository)) if err != nil { - return nil, err + return nil, nil, err } for rows.Next() { @@ -506,7 +506,7 @@ func (nl NotificationList) LoadRepos() (RepositoryList, error) { err = rows.Scan(&repo) if err != nil { rows.Close() - return nil, err + return nil, nil, err } repos[repo.ID] = &repo @@ -517,14 +517,21 @@ func (nl NotificationList) LoadRepos() (RepositoryList, error) { repoIDs = repoIDs[limit:] } + failed := []int{} + var reposList = make(RepositoryList, 0, len(repoIDs)) - for _, notification := range nl { + for i, notification := range nl { if notification.Repository == nil { notification.Repository = repos[notification.RepoID] } + if notification.Repository == nil { + log.Error("Notification[%d]: RepoID: %d not found", notification.ID, notification.RepoID) + failed = append(failed, i) + continue + } var found bool for _, r := range reposList { - if r.ID == notification.Repository.ID { + if r.ID == notification.RepoID { found = true break } @@ -533,7 +540,7 @@ func (nl NotificationList) LoadRepos() (RepositoryList, error) { reposList = append(reposList, notification.Repository) } } - return reposList, nil + return reposList, failed, nil } func (nl NotificationList) getPendingIssueIDs() []int64 { @@ -550,9 +557,9 @@ func (nl NotificationList) getPendingIssueIDs() []int64 { } // LoadIssues loads issues from database -func (nl NotificationList) LoadIssues() error { +func (nl NotificationList) LoadIssues() ([]int, error) { if len(nl) == 0 { - return nil + return []int{}, nil } var issueIDs = nl.getPendingIssueIDs() @@ -567,7 +574,7 @@ func (nl NotificationList) LoadIssues() error { In("id", issueIDs[:limit]). Rows(new(Issue)) if err != nil { - return err + return nil, err } for rows.Next() { @@ -575,7 +582,7 @@ func (nl NotificationList) LoadIssues() error { err = rows.Scan(&issue) if err != nil { rows.Close() - return err + return nil, err } issues[issue.ID] = &issue @@ -586,13 +593,38 @@ func (nl NotificationList) LoadIssues() error { issueIDs = issueIDs[limit:] } - for _, notification := range nl { + failures := []int{} + + for i, notification := range nl { if notification.Issue == nil { notification.Issue = issues[notification.IssueID] + if notification.Issue == nil { + log.Error("Notification[%d]: IssueID: %d Not Found", notification.ID, notification.IssueID) + failures = append(failures, i) + continue + } notification.Issue.Repo = notification.Repository } } - return nil + return failures, nil +} + +// Without returns the notification list without the failures +func (nl NotificationList) Without(failures []int) NotificationList { + if failures == nil || len(failures) == 0 { + return nl + } + remaining := make([]*Notification, 0, len(nl)) + last := -1 + var i int + for _, i = range failures { + remaining = append(remaining, nl[last+1:i]...) + last = i + } + if len(nl) > i { + remaining = append(remaining, nl[i+1:]...) + } + return remaining } func (nl NotificationList) getPendingCommentIDs() []int64 { @@ -609,9 +641,9 @@ func (nl NotificationList) getPendingCommentIDs() []int64 { } // LoadComments loads comments from database -func (nl NotificationList) LoadComments() error { +func (nl NotificationList) LoadComments() ([]int, error) { if len(nl) == 0 { - return nil + return []int{}, nil } var commentIDs = nl.getPendingCommentIDs() @@ -626,7 +658,7 @@ func (nl NotificationList) LoadComments() error { In("id", commentIDs[:limit]). Rows(new(Comment)) if err != nil { - return err + return nil, err } for rows.Next() { @@ -634,7 +666,7 @@ func (nl NotificationList) LoadComments() error { err = rows.Scan(&comment) if err != nil { rows.Close() - return err + return nil, err } comments[comment.ID] = &comment @@ -645,13 +677,19 @@ func (nl NotificationList) LoadComments() error { commentIDs = commentIDs[limit:] } - for _, notification := range nl { + failures := []int{} + for i, notification := range nl { if notification.CommentID > 0 && notification.Comment == nil && comments[notification.CommentID] != nil { notification.Comment = comments[notification.CommentID] + if notification.Comment == nil { + log.Error("Notification[%d]: CommentID[%d] failed to load", notification.ID, notification.CommentID) + failures = append(failures, i) + continue + } notification.Comment.Issue = notification.Issue } } - return nil + return failures, nil } // GetNotificationCount returns the notification count for user diff --git a/routers/user/notification.go b/routers/user/notification.go index d0ab3dbe8..74803f149 100644 --- a/routers/user/notification.go +++ b/routers/user/notification.go @@ -81,24 +81,39 @@ func Notifications(c *context.Context) { return } - repos, err := notifications.LoadRepos() + failCount := 0 + + repos, failures, err := notifications.LoadRepos() if err != nil { c.ServerError("LoadRepos", err) return } + notifications = notifications.Without(failures) if err := repos.LoadAttributes(); err != nil { c.ServerError("LoadAttributes", err) return } + failCount += len(failures) - if err := notifications.LoadIssues(); err != nil { + failures, err = notifications.LoadIssues() + if err != nil { c.ServerError("LoadIssues", err) return } - if err := notifications.LoadComments(); err != nil { + notifications = notifications.Without(failures) + failCount += len(failures) + + failures, err = notifications.LoadComments() + if err != nil { c.ServerError("LoadComments", err) return } + notifications = notifications.Without(failures) + failCount += len(failures) + + if failCount > 0 { + c.Flash.Error(fmt.Sprintf("ERROR: %d notifications were removed due to missing parts - check the logs", failCount)) + } title := c.Tr("notifications") if status == models.NotificationStatusUnread && total > 0 {