diff --git a/i18n/en.json b/i18n/en.json index 6ed8c3d6d6..40440f1919 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6910,14 +6910,6 @@ "id": "store.sql_reaction.delete.app_error", "translation": "Unable to delete reaction." }, - { - "id": "store.sql_reaction.delete.begin.app_error", - "translation": "Unable to open transaction while deleting reaction." - }, - { - "id": "store.sql_reaction.delete.commit.app_error", - "translation": "Unable to commit transaction while deleting reaction." - }, { "id": "store.sql_reaction.delete_all_with_emoji_name.delete_reactions.app_error", "translation": "Unable to delete all reactions with this emoji name." diff --git a/store/sqlstore/supplier_reactions.go b/store/sqlstore/supplier_reactions.go index 457e36750c..f0f76f5bf0 100644 --- a/store/sqlstore/supplier_reactions.go +++ b/store/sqlstore/supplier_reactions.go @@ -6,10 +6,12 @@ package sqlstore import ( "net/http" - "github.com/mattermost/gorp" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" + + "github.com/mattermost/gorp" + "github.com/pkg/errors" ) type SqlReactionStore struct { @@ -56,19 +58,24 @@ func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *mod } func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { - transaction, err := s.GetMaster().Begin() + err := store.WithDeadlockRetry(func() error { + transaction, err := s.GetMaster().Begin() + if err != nil { + return errors.Wrap(err, "begin_transaction") + } + defer finalizeTransaction(transaction) + + if err := deleteReactionAndUpdatePost(transaction, reaction); err != nil { + return errors.Wrap(err, "deleteReactionAndUpdatePost") + } + + if err := transaction.Commit(); err != nil { + return errors.Wrap(err, "commit_transaction") + } + return nil + }) if err != nil { - return nil, model.NewAppError("SqlReactionStore.Delete", "store.sql_reaction.delete.begin.app_error", nil, err.Error(), http.StatusInternalServerError) - } - defer finalizeTransaction(transaction) - - appErr := deleteReactionAndUpdatePost(transaction, reaction) - if appErr != nil { - return nil, model.NewAppError("SqlPreferenceStore.Delete", "store.sql_reaction.delete.app_error", nil, appErr.Error(), http.StatusInternalServerError) - } - - if err := transaction.Commit(); err != nil { - return nil, model.NewAppError("SqlPreferenceStore.Delete", "store.sql_reaction.delete.commit.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, model.NewAppError("SqlReactionStore.Delete", "store.sql_reaction.delete.app_error", nil, err.Error(), http.StatusInternalServerError) } return reaction, nil @@ -124,20 +131,34 @@ func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppEr "emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError) } - if _, err := s.GetMaster().Exec( - `DELETE FROM + err := store.WithDeadlockRetry(func() error { + _, err := s.GetMaster().Exec( + `DELETE FROM Reactions WHERE - EmojiName = :EmojiName`, map[string]interface{}{"EmojiName": emojiName}); err != nil { + EmojiName = :EmojiName`, map[string]interface{}{"EmojiName": emojiName}) + return err + }) + if err != nil { return model.NewAppError("SqlReactionStore.DeleteAllWithEmojiName", "store.sql_reaction.delete_all_with_emoji_name.delete_reactions.app_error", nil, "emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError) } for _, reaction := range reactions { - if _, err := s.GetMaster().Exec(UPDATE_POST_HAS_REACTIONS_ON_DELETE_QUERY, - map[string]interface{}{"PostId": reaction.PostId, "UpdateAt": model.GetMillis()}); err != nil { - mlog.Warn("Unable to update Post.HasReactions while removing reactions", mlog.String("post_id", reaction.PostId), mlog.Err(err)) + reaction := reaction + err := store.WithDeadlockRetry(func() error { + _, err := s.GetMaster().Exec(UPDATE_POST_HAS_REACTIONS_ON_DELETE_QUERY, + map[string]interface{}{ + "PostId": reaction.PostId, + "UpdateAt": model.GetMillis(), + }) + return err + }) + if err != nil { + mlog.Warn("Unable to update Post.HasReactions while removing reactions", + mlog.String("post_id", reaction.PostId), + mlog.Err(err)) } } diff --git a/store/storetest/reaction_store.go b/store/storetest/reaction_store.go index 72625f3d83..cbc72dc2d6 100644 --- a/store/storetest/reaction_store.go +++ b/store/storetest/reaction_store.go @@ -4,10 +4,12 @@ package storetest import ( + "sync" "testing" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -19,6 +21,7 @@ func TestReactionStore(t *testing.T, ss store.Store) { t.Run("ReactionDeleteAllWithEmojiName", func(t *testing.T) { testReactionDeleteAllWithEmojiName(t, ss) }) t.Run("PermanentDeleteBatch", func(t *testing.T) { testReactionStorePermanentDeleteBatch(t, ss) }) t.Run("ReactionBulkGetForPosts", func(t *testing.T) { testReactionBulkGetForPosts(t, ss) }) + t.Run("ReactionDeadlock", func(t *testing.T) { testReactionDeadlock(t, ss) }) } func testReactionSave(t *testing.T, ss store.Store) { @@ -95,7 +98,6 @@ func testReactionSave(t *testing.T, ss store.Store) { } _, err = ss.Reaction().Save(reaction5) require.NotNil(t, err, "should've failed for invalid reaction") - } func testReactionDelete(t *testing.T, ss store.Store) { @@ -422,3 +424,65 @@ func testReactionBulkGetForPosts(t *testing.T, ss store.Store) { require.False(t, post4IdFound, "Wrong reaction returned") } + +// testReactionDeadlock is a best-case attempt to recreate the deadlock scenario. +// It at least deadlocks 2 times out of 5. +func testReactionDeadlock(t *testing.T, ss store.Store) { + post, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + }) + require.Nil(t, err) + + reaction1 := &model.Reaction{ + UserId: model.NewId(), + PostId: post.Id, + EmojiName: model.NewId(), + } + _, err = ss.Reaction().Save(reaction1) + require.Nil(t, err) + + // different user + reaction2 := &model.Reaction{ + UserId: model.NewId(), + PostId: reaction1.PostId, + EmojiName: reaction1.EmojiName, + } + _, err = ss.Reaction().Save(reaction2) + require.Nil(t, err) + + // different post + reaction3 := &model.Reaction{ + UserId: reaction1.UserId, + PostId: model.NewId(), + EmojiName: reaction1.EmojiName, + } + _, err = ss.Reaction().Save(reaction3) + require.Nil(t, err) + + // different emoji + reaction4 := &model.Reaction{ + UserId: reaction1.UserId, + PostId: reaction1.PostId, + EmojiName: model.NewId(), + } + _, err = ss.Reaction().Save(reaction4) + require.Nil(t, err) + + var wg sync.WaitGroup + wg.Add(2) + // 1st tx + go func() { + defer wg.Done() + err := ss.Reaction().DeleteAllWithEmojiName(reaction1.EmojiName) + require.Nil(t, err) + }() + + // 2nd tx + go func() { + defer wg.Done() + _, err := ss.Reaction().Delete(reaction2) + require.Nil(t, err) + }() + wg.Wait() +}