diff --git a/src/github.com/matrix-org/dendrite/common/sql.go b/src/github.com/matrix-org/dendrite/common/sql.go index cabbe6662..4abe7410e 100644 --- a/src/github.com/matrix-org/dendrite/common/sql.go +++ b/src/github.com/matrix-org/dendrite/common/sql.go @@ -18,6 +18,24 @@ import ( "database/sql" ) +// A Transaction is something that can be committed or rolledback. +type Transaction interface { + // Commit the transaction + Commit() error + // Rollback the transaction. + Rollback() error +} + +// EndTransaction ends a transaction. +// If the transaction succeeded then it is committed, otherwise it is rolledback. +func EndTransaction(txn Transaction, succeeded *bool) { + if *succeeded { + txn.Commit() + } else { + txn.Rollback() + } +} + // WithTransaction runs a block of code passing in an SQL transaction // If the code returns an error or panics then the transactions is rolledback // Otherwise the transaction is committed. @@ -26,16 +44,14 @@ func WithTransaction(db *sql.DB, fn func(txn *sql.Tx) error) (err error) { if err != nil { return } - defer func() { - if r := recover(); r != nil { - txn.Rollback() - panic(r) - } else if err != nil { - txn.Rollback() - } else { - err = txn.Commit() - } - }() + succeeded := false + defer EndTransaction(txn, &succeeded) + err = fn(txn) + if err != nil { + return + } + + succeeded = true return } diff --git a/src/github.com/matrix-org/dendrite/roomserver/input/events.go b/src/github.com/matrix-org/dendrite/roomserver/input/events.go index c16de5e6f..82b4652e6 100644 --- a/src/github.com/matrix-org/dendrite/roomserver/input/events.go +++ b/src/github.com/matrix-org/dendrite/roomserver/input/events.go @@ -17,6 +17,7 @@ package input import ( "fmt" + "github.com/matrix-org/dendrite/common" "github.com/matrix-org/dendrite/roomserver/api" "github.com/matrix-org/dendrite/roomserver/state" "github.com/matrix-org/dendrite/roomserver/types" @@ -122,69 +123,49 @@ func processInviteEvent(db RoomEventDatabase, ow OutputRoomEventWriter, input ap if err != nil { return err } + succeeded := false + defer common.EndTransaction(updater, &succeeded) - return withTransaction(updater, func() error { + if updater.IsJoin() { + // If the user is joined to the room then that takes precedence over this + // invite event. It makes little sense to move a user that is already + // joined to the room into the invite state. + // This could plausibly happen if an invite request raced with a join + // request for a user. For example if a user was invited to a public + // room and they joined the room at the same time as the invite was sent. + // The other way this could plausibly happen is if an invite raced with + // a kick. For example if a user was kicked from a room in error and in + // response someone else in the room re-invited them then it is possible + // for the invite request to race with the leave event so that the + // target receives invite before it learns that it has been kicked. + // There are a few ways this could be plausibly handled in the roomserver. + // 1) Store the invite, but mark it as retired. That will result in the + // permanent rejection of that invite event. So even if the target + // user leaves the room and the invite is retransmitted it will be + // ignored. However a new invite with a new event ID would still be + // accepted. + // 2) Silently discard the invite event. This means that if the event + // was retransmitted at a later date after the target user had left + // the room we would accept the invite. However since we hadn't told + // the sending server that the invite had been discarded it would + // have no reason to attempt to retry. + // 3) Signal the sending server that the user is already joined to the + // room. + // For now we will implement option 2. Since in the abesence of a retry + // mechanism it will be equivalent to option 1, and we don't have a + // signalling mechanism to implement option 3. + return nil + } - if updater.IsJoin() { - // If the user is joined to the room then that takes precedence over this - // invite event. It makes little sense to move a user that is already - // joined to the room into the invite state. - // This could plausibly happen if an invite request raced with a join - // request for a user. For example if a user was invited to a public - // room and they joined the room at the same time as the invite was sent. - // The other way this could plausibly happen is if an invite raced with - // a kick. For example if a user was kicked from a room in error and in - // response someone else in the room re-invited them then it is possible - // for the invite request to race with the leave event so that the - // target receives invite before it learns that it has been kicked. - // There are a few ways this could be plausibly handled in the roomserver. - // 1) Store the invite, but mark it as retired. That will result in the - // permanent rejection of that invite event. So even if the target - // user leaves the room and the invite is retransmitted it will be - // ignored. However a new invite with a new event ID would still be - // accepted. - // 2) Silently discard the invite event. This means that if the event - // was retransmitted at a later date after the target user had left - // the room we would accept the invite. However since we hadn't told - // the sending server that the invite had been discarded it would - // have no reason to attempt to retry. - // 3) Signal the sending server that the user is already joined to the - // room. - // For now we will implement option 2. Since in the abesence of a retry - // mechanism it will be equivalent to option 1, and we don't have a - // signalling mechanism to implement option 3. - return nil - } + outputUpdates, err := updateToInviteMembership(updater, &input.Event, nil) + if err != nil { + return err + } - outputUpdates, err := updateToInviteMembership(updater, &input.Event, nil) - if err != nil { - return err - } + if err = ow.WriteOutputEvents(roomID, outputUpdates); err != nil { + return err + } - return ow.WriteOutputEvents(roomID, outputUpdates) - }) -} - -// withTransaction runs a function inside a transaction. -// Commits the transaction if the function succeeds. -// Rollsback the transaction if the function fails or panics. -func withTransaction(t types.Transaction, f func() error) (err error) { - defer func() { - if r := recover(); r != nil { - t.Rollback() - panic(r) - } - if err != nil { - // Ignore any error we get rolling back since we don't want to - // clobber the current error - // TODO: log the error here. - t.Rollback() - } else { - // Commit if there wasn't an error. - // Set the returned err value if we encounter an error committing. - // This only works because err is a named return. - err = t.Commit() - } - }() - return f() + succeeded = true + return nil } diff --git a/src/github.com/matrix-org/dendrite/roomserver/input/latest_events.go b/src/github.com/matrix-org/dendrite/roomserver/input/latest_events.go index dce1775f6..d9aa2b455 100644 --- a/src/github.com/matrix-org/dendrite/roomserver/input/latest_events.go +++ b/src/github.com/matrix-org/dendrite/roomserver/input/latest_events.go @@ -17,6 +17,7 @@ package input import ( "bytes" + "github.com/matrix-org/dendrite/common" "github.com/matrix-org/dendrite/roomserver/api" "github.com/matrix-org/dendrite/roomserver/state" "github.com/matrix-org/dendrite/roomserver/types" @@ -52,12 +53,19 @@ func updateLatestEvents( if err != nil { return } + succeeded := false + defer common.EndTransaction(updater, &succeeded) u := latestEventsUpdater{ db: db, updater: updater, ow: ow, roomNID: roomNID, stateAtEvent: stateAtEvent, event: event, sendAsServer: sendAsServer, } - return withTransaction(updater, u.doUpdateLatestEvents) + if err = u.doUpdateLatestEvents(); err != nil { + return err + } + + succeeded = true + return } // latestEventsUpdater tracks the state used to update the latest events in the