Go vet will report this as a problem. > On Dec 3, 2018, at 5:54 PM, Ben Hoyt <benh...@gmail.com> wrote: > > Robert, here is code (actual working code this time) similar to what we have: > https://play.golang.org/p/jUPqgnk6Ttk > > Leonel, yep, I understand that, which is why we have this problem. The thing > is, this pattern makes it very easy to forget to always re-use the same error > variable, especially in code that's a bit more complex with a bunch of nested > ifs, etc. So we're trying to come up with a pattern that's hard or impossible > to misuse, instead of easy to misuse. > > -Ben > >> On Mon, Dec 3, 2018 at 6:46 PM Leonel Quinteros <leonel.quinte...@gmail.com> >> wrote: >> 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 a topic in the >> Google Groups "golang-nuts" group. >> To unsubscribe from this topic, visit >> https://groups.google.com/d/topic/golang-nuts/ge49ywYnjio/unsubscribe. >> To unsubscribe from this group and all its topics, send an email to >> golang-nuts+unsubscr...@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. > > -- > 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.
-- 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.
