diff --git a/store/sqlstore/channel_store_categories.go b/store/sqlstore/channel_store_categories.go index 8a978fd55f..2f4fd8bd04 100644 --- a/store/sqlstore/channel_store_categories.go +++ b/store/sqlstore/channel_store_categories.go @@ -640,6 +640,11 @@ func (s SqlChannelStore) UpdateSidebarCategories(userId, teamId string, categori updatedCategory.Muted = category.Muted } + // The order in which the queries are executed in the transaction is important. + // SidebarCategories need to be update first, and then SidebarChannels should be deleted. + // The net effect remains the same, but it prevents deadlocks from other transactions + // operating on the tables in reverse order. + updateQuery, updateParams, _ := s.getQueryBuilder(). Update("SidebarCategories"). Set("DisplayName", updatedCategory.DisplayName). @@ -1002,30 +1007,33 @@ func (s SqlChannelStore) DeleteSidebarCategory(categoryId string) error { return store.NewErrInvalidInput("SidebarCategory", "id", categoryId) } - // Delete the channels in the category - query, args, err := s.getQueryBuilder(). - Delete("SidebarChannels"). - Where(sq.Eq{"CategoryId": categoryId}).ToSql() - if err != nil { - return errors.Wrap(err, "delete_sidebar_cateory_tosql") - } - - if _, err = transaction.Exec(query, args...); err != nil { - return errors.Wrap(err, "failed to delete SidebarChannel") - } + // The order in which the queries are executed in the transaction is important. + // SidebarCategories need to be deleted first, and then SidebarChannels. + // The net effect remains the same, but it prevents deadlocks from other transactions + // operating on the tables in reverse order. // Delete the category itself - query, args, err = s.getQueryBuilder(). + query, args, err := s.getQueryBuilder(). Delete("SidebarCategories"). Where(sq.Eq{"Id": categoryId}).ToSql() if err != nil { return errors.Wrap(err, "delete_sidebar_cateory_tosql") } - if _, err = transaction.Exec(query, args...); err != nil { return errors.Wrap(err, "failed to delete SidebarCategory") } + // Delete the channels in the category + query, args, err = s.getQueryBuilder(). + Delete("SidebarChannels"). + Where(sq.Eq{"CategoryId": categoryId}).ToSql() + if err != nil { + return errors.Wrap(err, "delete_sidebar_cateory_tosql") + } + if _, err = transaction.Exec(query, args...); err != nil { + return errors.Wrap(err, "failed to delete SidebarChannel") + } + if err := transaction.Commit(); err != nil { return errors.Wrap(err, "commit_transaction") } diff --git a/store/storetest/channel_store_categories.go b/store/storetest/channel_store_categories.go index 410db37e90..e4c0df8690 100644 --- a/store/storetest/channel_store_categories.go +++ b/store/storetest/channel_store_categories.go @@ -23,9 +23,9 @@ func TestChannelStoreCategories(t *testing.T, ss store.Store, s SqlStore) { t.Run("GetSidebarCategories", func(t *testing.T) { testGetSidebarCategories(t, ss) }) t.Run("UpdateSidebarCategories", func(t *testing.T) { testUpdateSidebarCategories(t, ss) }) t.Run("ClearSidebarOnTeamLeave", func(t *testing.T) { testClearSidebarOnTeamLeave(t, ss, s) }) - t.Run("UpdateSidebarCategories", func(t *testing.T) { testUpdateSidebarCategories(t, ss) }) t.Run("DeleteSidebarCategory", func(t *testing.T) { testDeleteSidebarCategory(t, ss, s) }) t.Run("UpdateSidebarChannelsByPreferences", func(t *testing.T) { testUpdateSidebarChannelsByPreferences(t, ss) }) + t.Run("SidebarCategoryDeadlock", func(t *testing.T) { testSidebarCategoryDeadlock(t, ss) }) } func testCreateInitialSidebarCategories(t *testing.T, ss store.Store) { @@ -2041,3 +2041,68 @@ func testUpdateSidebarChannelsByPreferences(t *testing.T, ss store.Store) { }) }) } + +// testSidebarCategoryDeadlock tries to delete and update a category at the same time +// in the hope of triggering a deadlock. This is a best-effort test case, and is not guaranteed +// to catch a bug. +func testSidebarCategoryDeadlock(t *testing.T, ss store.Store) { + userID := model.NewId() + teamID := model.NewId() + + // Join a channel + channel, err := ss.Channel().Save(&model.Channel{ + Name: "channel", + Type: model.CHANNEL_OPEN, + TeamId: teamID, + }, 10) + require.NoError(t, err) + _, err = ss.Channel().SaveMember(&model.ChannelMember{ + UserId: userID, + ChannelId: channel.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) + + // And then create the initial categories so that it includes the channel + res, err := ss.Channel().CreateInitialSidebarCategories(userID, teamID) + require.NoError(t, err) + require.NotEmpty(t, res) + + initialCategories, err := ss.Channel().GetSidebarCategories(userID, teamID) + require.NoError(t, err) + + channelsCategory := initialCategories.Categories[1] + require.Equal(t, []string{channel.Id}, channelsCategory.Channels) + + customCategory, err := ss.Channel().CreateSidebarCategory(userID, teamID, &model.SidebarCategoryWithChannels{}) + require.NoError(t, err) + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + _, _, err := ss.Channel().UpdateSidebarCategories(userID, teamID, []*model.SidebarCategoryWithChannels{ + { + SidebarCategory: channelsCategory.SidebarCategory, + Channels: []string{}, + }, + { + SidebarCategory: customCategory.SidebarCategory, + Channels: []string{channel.Id}, + }, + }) + if err != nil { + var nfErr *store.ErrNotFound + require.True(t, errors.As(err, &nfErr)) + } + }() + + go func() { + defer wg.Done() + err := ss.Channel().DeleteSidebarCategory(customCategory.Id) + require.NoError(t, err) + }() + + wg.Wait() +}