diff --git a/app/channel.go b/app/channel.go index 0c92f57d2b..b01a7092c0 100644 --- a/app/channel.go +++ b/app/channel.go @@ -229,7 +229,7 @@ func (a *App) CreateChannel(channel *model.Channel, addMember bool) (*model.Chan return nil, model.NewAppError("CreateChannel", "store.sql_channel.save_channel.existing.app_error", nil, "id="+invErr.Value.(string), http.StatusBadRequest) } case errors.As(nErr, &cErr): - return channel, model.NewAppError("CreateChannel", store.CHANNEL_EXISTS_ERROR, nil, cErr.Error(), http.StatusBadRequest) + return sc, model.NewAppError("CreateChannel", store.CHANNEL_EXISTS_ERROR, nil, cErr.Error(), http.StatusBadRequest) case errors.As(nErr, <Err): return nil, model.NewAppError("CreateChannel", "store.sql_channel.save_channel.limit.app_error", nil, ltErr.Error(), http.StatusBadRequest) case errors.As(nErr, &appErr): // in case we haven't converted to plain error. diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index a0e15234be..f546ec8e8a 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -518,27 +518,32 @@ func (s SqlChannelStore) Save(channel *model.Channel, maxChannelsPerTeam int64) return nil, store.NewErrInvalidInput("Channel", "Type", channel.Type) } - transaction, err := s.GetMaster().Begin() - if err != nil { - return nil, errors.Wrapf(err, "begin_transaction: ") - } - defer finalizeTransaction(transaction) + var newChannel *model.Channel + err := store.WithDeadlockRetry(func() error { + transaction, err := s.GetMaster().Begin() + if err != nil { + return errors.Wrap(err, "begin_transaction") + } + defer finalizeTransaction(transaction) - newChannel, err := s.saveChannelT(transaction, channel, maxChannelsPerTeam) - if err != nil { - return newChannel, err - } + newChannel, err = s.saveChannelT(transaction, channel, maxChannelsPerTeam) + if err != nil { + return err + } - // Additionally propagate the write to the PublicChannels table. - if err := s.upsertPublicChannelT(transaction, newChannel); err != nil { - return nil, errors.Wrapf(err, "upsert_public_channel: ") - } + // Additionally propagate the write to the PublicChannels table. + if err := s.upsertPublicChannelT(transaction, newChannel); err != nil { + return errors.Wrap(err, "upsert_public_channel") + } - if err := transaction.Commit(); err != nil { - return nil, errors.Wrapf(err, "commit_transaction: ") - } - - return newChannel, nil + if err := transaction.Commit(); err != nil { + return errors.Wrap(err, "commit_transaction") + } + return nil + }) + // There are cases when in case of conflict, the original channel value is returned. + // So we return both and let the caller do the checks. + return newChannel, err } func (s SqlChannelStore) CreateDirectChannel(user *model.User, otherUser *model.User) (*model.Channel, error) { diff --git a/store/store.go b/store/store.go index 84fb674bb2..32e121b80d 100644 --- a/store/store.go +++ b/store/store.go @@ -9,7 +9,11 @@ import ( "context" "time" + "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" + + "github.com/go-sql-driver/mysql" + "github.com/pkg/errors" ) type StoreResult struct { @@ -770,3 +774,31 @@ type IntegrityCheckResult struct { Data interface{} Err error } + +const mySQLDeadlockCode = uint16(1213) + +// WithDeadlockRetry retries a given f if it throws a deadlock error. +// It breaks after a threshold and propagates the error upwards. +// TODO: This can be a separate retry layer in itself where transaction retries +// are automatically applied. +func WithDeadlockRetry(f func() error) error { + var err error + for i := 0; i < 3; i++ { + err = f() + if err == nil { + // No error, return nil. + return nil + } + // XXX: Possibly add check for postgres deadlocks later. + // But deadlocks are very rarely seen in postgres. + var mysqlErr *mysql.MySQLError + if errors.As(err, &mysqlErr) && mysqlErr.Number == mySQLDeadlockCode { + mlog.Warn("A deadlock happened. Retrying.", mlog.Err(err)) + // This is a deadlock, retry. + continue + } + // Some other error, return as-is. + return err + } + return errors.Wrap(err, "giving up after 3 consecutive deadlocks") +} diff --git a/store/store_test.go b/store/store_test.go new file mode 100644 index 0000000000..bf64a28ace --- /dev/null +++ b/store/store_test.go @@ -0,0 +1,56 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package store + +import ( + "testing" + + "github.com/go-sql-driver/mysql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type deadlock struct { + numRetries int + hasRetried int +} + +func newDeadlock(numRetries int) *deadlock { + return &deadlock{ + numRetries: numRetries, + } +} + +func (d *deadlock) f() error { + if d.numRetries == d.hasRetried { + return nil + } + d.hasRetried++ + return &mysql.MySQLError{ + Number: mySQLDeadlockCode, + } +} + +func TestDeadlockRetry(t *testing.T) { + t.Run("NoDeadlock", func(t *testing.T) { + d := newDeadlock(0) + err := WithDeadlockRetry(d.f) + require.NoError(t, err) + assert.Equal(t, 0, d.hasRetried) + }) + + t.Run("1Deadlock", func(t *testing.T) { + d := newDeadlock(1) + err := WithDeadlockRetry(d.f) + require.NoError(t, err) + assert.Equal(t, 1, d.hasRetried) + }) + + t.Run("AlwaysDeadlock", func(t *testing.T) { + d := newDeadlock(4) + err := WithDeadlockRetry(d.f) + require.Error(t, err) + assert.Equal(t, 3, d.hasRetried) + }) +}