transaction blocks: don't swallow panics (#2774)

This improves upon #2767.

Previously, the code would swallow any panics, which isn't ideal;
panic is intended to be used when a critical error arises,
where the process should fail fast instead of trying to limp along.

This now defers the any recovery (if desired) to the client code.

main.go

@@ -528,12 +528,12 @@ func (s *DB) Debug() *DB {
 // Transaction start a transaction as a block,
 // return error will rollback, otherwise to commit.
 func (s *DB) Transaction(fc func(tx *DB) error) (err error) {
+	panicked := true
 	tx := s.Begin()
 	defer func() {
-		if r := recover(); r != nil {
-			err = fmt.Errorf("%s", r)
+		// Make sure to rollback when panic, Block error or Commit error
+		if panicked || err != nil {
 			tx.Rollback()
-			return
 		}
 	}()
 
@@ -543,10 +543,7 @@ func (s *DB) Transaction(fc func(tx *DB) error) (err error) {
 		err = tx.Commit().Error
 	}
 
-	// Makesure rollback when Block error or Commit error
-	if err != nil {
-		tx.Rollback()
-	}
+	panicked = false
 	return
 }
 

main_test.go

@@ -470,6 +470,15 @@ func TestTransaction(t *testing.T) {
 	}
 }
 
+func assertPanic(t *testing.T, f func()) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("The code did not panic")
+		}
+	}()
+	f()
+}
+
 func TestTransactionWithBlock(t *testing.T) {
 	// rollback
 	err := DB.Transaction(func(tx *gorm.DB) error {
@@ -511,6 +520,7 @@ func TestTransactionWithBlock(t *testing.T) {
 	}
 
 	// panic will rollback
+	assertPanic(t, func() {
 		DB.Transaction(func(tx *gorm.DB) error {
 			u3 := User{Name: "transcation-3"}
 			if err := tx.Save(&u3).Error; err != nil {
@@ -523,6 +533,7 @@ func TestTransactionWithBlock(t *testing.T) {
 
 			panic("force panic")
 		})
+	})
 
 	if err := DB.First(&User{}, "name = ?", "transcation").Error; err == nil {
 		t.Errorf("Should not find record after panic rollback")