Noisy log message transaction has already been committed or rolled back during regular usage

[j16r]

Since we use this pattern:

tx.begin()
defer tx.rollback()

This means rollback is called even when a transaction is committed. This log message is mostly harmless, but it makes it difficult to properly diagnose improper use of transactions in our code base.

Other than doing:

tx.begin()
if err != nil {
  tx.rollback()
  return err
}
return tx.commit()

Maybe this could be handled by suppressing logging from gormigrate's use of rollback or by checking if a commit has already been run and turning rollback into a no-op. Thoughts?

[andreynering]

Hi @j16r,

This pattern of using defer for rollback is permitted by the standard library's database/sql package. It's intended to be used like this. It just returns sql.ErrTxDone without any side effects.

So it's sad that Gorm doesn't recognize this pattern. But I think it'd be easy to fix it here and here.

From

s.AddError(db.Rollback())

To

if err := db.Rollback(); err != nil && err != sql.TxDone {
	s.AddError(err)
}

What do you think? We could propose a PR to Gorm, and in case they don't accept it, we could do some logic on our side to keep track of the transaction state.

[j16r]

Sounds good, there are some other logging issues I'd like to address in gorm so I'll issue a PR there.

[andreynering]

@j16r Can this be closed now? 🙂