Use a nicer pattern for wrapping transactions

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() {
+	succeeded := false
-		if r := recover(); r != nil {
+	defer EndTransaction(txn, &succeeded)
-			txn.Rollback()
+
-			panic(r)
-		} else if err != nil {
-			txn.Rollback()
-		} else {
-			err = txn.Commit()
-		}
-	}()
 	err = fn(txn)
+	if err != nil {
+		return
+	}
+
+	succeeded = true
 	return
 }

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() {
+	outputUpdates, err := updateToInviteMembership(updater, &input.Event, nil)
-			// If the user is joined to the room then that takes precedence over this
+	if err != nil {
-			// invite event. It makes little sense to move a user that is already
+		return err
-			// 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 = ow.WriteOutputEvents(roomID, outputUpdates); err != nil {
-		if err != nil {
+		return err
-			return err
+	}
-		}
 
-		return ow.WriteOutputEvents(roomID, outputUpdates)
+	succeeded = true
-	})
+	return nil
-}
-
-// 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()
 }

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