Hi Ben,
I'm pretty sure that the *err* variable is getting shadowed on your Update
construct.
Instead of doing:
if _, err := UpdateBar(tx); err != nil {
return err
}
You should do something like:
_, err = UpdateBar(tx)
if err != nil {
return err
}
Just like you do with the insert and avoiding the *:=* operand
Let me know if that works.
Best!
Leonel
El lunes, 3 de diciembre de 2018, 18:53:30 (UTC-3), Ben Hoyt escribió:
>
> 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.
