The best way to access the return value of a function is to name the return
variable:
func DoTwoThings(db *Database) (err error) {
tx, err := db.Begin()
if err != nil {
return err
}
defer tx.Close(&err)
...
}
Then any accidental shadowing doesn't matter because the "defer" is
referring to the actual return variable; it also means that you can support
returning a new error directly (i.e. `return nil, fmt.Errorf(...)`) rather
than needing to set the outer 'err' variable and then return it.
On Monday, December 3, 2018 at 3:55:03 PM UTC-8, Ben Hoyt 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.q...@gmail.com
> <javascript:>> 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...@googlegroups.com <javascript:>.
>> 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.
