Helpful comments and good suggestion, thanks Leonel. -Ben On Tue, Dec 4, 2018 at 9:15 AM Leonel Quinteros <leonel.quinte...@gmail.com> wrote:
> Hi Ben, > > Sorry, I've misread that part. > Now that I got some sleep and reviewed your code a little bit more, it > looks like you got trapped by your own design decisions. > > First of all, variable shadowing is something every Go developer should be > able to handle gracefully, but I understand your concern about how easy > it's to miss that bug. Doesn't the linter alert you about that shadowing > process? Just curious about that. > > Then, the mistake, IMHO, is the use of the defer function and the custom > tx.Close method (is it custom made, right? I haven't found a reference for > that method in the sql.Tx type docs) that "saves" you from manually > handling tx.Commit() vs tx.Rollback() calls, which if you think, it gets > pretty well aligned to the error handling philosophy of Go. If you get an > error, please go ahead and explicitly handle that error. > > So, your DoTwoThings() function is actually doing 3 things, the 2 things > against the DB, but it's also handling the transaction. If you see these 2 > DB queries as a unit, you may want to put these in a method that takes a > sql.Tx object, executes queries in that context and return an error result > that will indicate if all of them were successful or not. Then you Commit > or Rollback the transaction in the caller, not in a deeper level. > So pretty much as your Transact() method, but I'm not a fan of the defer > use there, while I get how you managed to also catch panics there. > Out of your 2 proposals, I like this one better. > > But if you ask me, I'd recommend to avoid the defer use, which is the > thing that's actually causing the shadowing problem, and to enforce the > manually handling of the transactions directly related to error handling. > That's how I've been doing it and it's pretty clear and straightforward > when you read that code, the Rollbacks after the errors and the Commits at > the end of each function are just there, explicitly telling you what's > happening when some DB query fails. > > But again, it's a matter of opinion, design and personal preferences. > I just hope this helps for you to form an opinion. > > Best! > > > *Leonel Quinteros* > Director of Technology > Global Brigades <https://www.globalbrigades.org/> > > > > El lun., 3 de dic. de 2018 a la(s) 20:54, Ben Hoyt (benh...@gmail.com) > escribió: > >> 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.
