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.
