In this situation, we normally use named return variables. That way we can
just check if we are returning with an error in the defer.
func DoTwoThings() (retErr error) {
tx, err := db.Begin()
if err != nil {
return err
}
defer func() {
if retErr != nil {
tx.Rollback()
}
}()
...
}
Note that this doesn't account for panics.
On Monday, December 3, 2018 at 4:53:30 PM UTC-5, Ben Hoyt wrote:
>
> 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.
