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.
