Hi folks,
We found some subtle bugs in our db transaction code for handling
commits/rollbacks. Here's the pattern we were using (not real, but shows
the issue):
func DoTwoThings() error {
tx, err := db.Begin()
if err != nil {
return err
}
// commit or rollback the transaction before we return
defer tx.Close(&err)
err := InsertFoo(tx)
if err != nil {
return err
}
if _, err := UpdateBar(tx); err != nil {
return err
}
return nil
}
The problem is there's a subtle but potentially quite bad bug with this
usage pattern -- if the InsertFoo succeeds but UpdateBar fails, the first
"err" variable will be nil, so the deferred tx.Close() will COMMIT the
transaction rather than ROLLBACK, and the database will be in an
inconsistent state.
The code above is a bit contrived, and you can easily fix it by moving the
"_, err := UpdateBar()" outside of the if so it's referring to the same
"err" variable, but it's very easy to miss and get it wrong. So we decided
it was a bad pattern and started thinking about the best way to fix.
One idea is a RollbackUnlessCommitted() function which you can defer, and
then you call Commit() once manually (stolen from gocraft/dbr):
func DoTwoThings() error {
tx, err := db.Begin()
if err != nil {
return err
}
defer tx.RollbackUnlessCommitted()
err := InsertFoo(tx)
if err != nil {
return err
}
if _, err := UpdateBar(tx); err != nil {
return err
}
tx.Commit()
return nil
}
Another idea is to create a "Transact" function which takes an anonymous
function and does all the transaction handling:
func (db *DatabaseImpl) Transact(txFunc func() error) (err error) {
tx, err := db.Begin()
if err != nil {
return
}
defer func() {
if p := recover(); p != nil {
tx.Rollback()
panic(p) // re-throw panic after Rollback
} else if err != nil {
tx.Rollback() // err is non-nil; don't change it
} else {
err = tx.Commit() // err is nil; if Commit returns error update
err
}
}()
err = txFunc(tx)
return err
}
And then the DoTwoThings function becomes:
func DoTwoThings() error {
return db.Transact(func() error) {
err := InsertFoo(tx)
if err != nil {
return err
}
if _, err := UpdateBar(tx); err != nil {
return err
}
})
}
I think the second is probably safer and nicer, but it's slightly awkward
in that it requires an extra level of indentation. Still, awkward is better
than buggy.
Does anyone else have a better pattern for this kind of thing, or feedback
on the above?
-Ben
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
