Transaction block returns nil

[kschiess]

This transaction block always returns nil:

transaction do
  return true
end

I was expecting true.

The code at https://github.com/datamapper/dm-transactions/blob/master/lib/dm-transactions.rb#L142 seems guilty: Return value is not assigned (because we return early) and is thus nil.

I would leave the return value alone, the block is at the end of the method anyway.

[jpr5]

@kschiess Keep in mind, closures aren't methods and you shouldn't return'' from a closure -- usenext'' instead.

If you must though, see the solution referenced in the Closures section at http://www.darkridge.com/~jpr5/2012/10/03/ruby-1.8.7-1.9.3-migration/.

cheers,
--jordan

[kschiess]

Not the problem at all - I am doing this inside a method and was expecting the return true .. to return true from the method. Don't tell me that's not possible. Here's what I really tried:

def foo
  transaction do
    return true
  end
end 

The code in dm-transactions turns this into a nil every time, which I consider a bug.

[jpr5]

Workaround:

def foo
  return transaction do
     next true
  end
end

Background: The issue came down to whether DM should Just Work by transparently correcting for a fairly common mistake made by programmers working within large (sometimes nested) transactional blocks, or should it be pedantic and insist on adhering to an unfortunate behavioral variance that many Ruby programmers finger-fumble at least once and most avoid doing entirely.

Without the hack, the consequence was bad: the transaction would never exit and quickly hang any other concurrent processes touching the same table. This silent, non-obvious fail would require SQL log enablement+scrutiny and/or DM code debugging (which is not an altogether fun experience). So we favored saving the user pain by catching the mistake, knowing there was an easy workaround.

Granted, in 1.9.3/2.0 the hack doesn't catch the return value, and that's a bug - hence the reference to the Ruby migration blog entry for a solution. I haven't implemented it yet because the solution feels heavy-handed; but that is probably a nonsense sentiment since the method works just fine for Sinatra handling high load stuff (though it doesn't create a new class each time).

[kschiess]

I admit that I don't understand the above - in my perhaps simpler world, taking out the return value handling would return the correct value, since the begin/end is the last statement in the method. But I assume that other Ruby versions and/or other use cases complicate this.

As far as I can tell, you talk mainly about committing when a return is called? I guess that makes sense, though I would side with a rollback when returning from the block early, but hey, we'd need to settle for something and document it I guess.

[jpr5]

My point was that next and return do different things between methods and closures, and that it is very easy and common for programmers to (accidentally) use return instead of next. This is especially true given the tendency for DM transactional contexts to get larger over time and the potential for nesting complexity, plus the fact that DM often spans many indirect code-paths that are closure-based (think filters, validations, etc). Tracking flow and keeping an eye out for an errant return gets Hard. Then consider the potential consequences on data integrity, production operations, and the programmer for having to hunt this down. The better alternative is to Just Work: make return inside a closure work like next, at least in the limited context of transactions.

For a really good primer on Ruby closures, see: http://innig.net/software/ruby/closures-in-ruby

WRT behaviors on return, the problem is that we can't know what the caller/closure intended. We can't distinguish a closure exit as being from next, return or full completion (other than with this hack). Neither would an interpretation of return values be consistently correct - when should it commit, when should it rollback? Ruby does prescribe exceptions for distinguishing out of band behaviors - which the hack still allows, and implies a rollback - but using them as a general early-exit mechanism would violate their OOB nature in the case where a raised exception is supposed to complete the transaction instead of abort it.

And FWIW, using exceptions to break out of transactional closures looks nasty already, adding extra indentation all over the place: in almost every case you have to wrap each transaction do ... end block with an outer begin ... rescue ... end block. Nest those transactions and it gets ridiculous fast. But, there's no other way to abort a transaction from within the closure and return code execution to whatever immediately follows the closure invocation. Extending this to early-exit with commit would look terrible.

A solution that (1) keeps the return safety mechanism and (2) returns the proper value on both 1.8.7 and 1.9.3/2.0 would be to create a (anonymous) class instance, bind the transaction closure to it as a method, and call it. This is essentially the solution referenced in the blog, and is also what Sinatra does to support return within its DSL.

[kschiess]

I am not sure we're talking about the same thing anymore. The specification at https://gist.github.com/kschiess/5865739 highlights (if run with 1.9.3) where I would expect the current implementation to perform differently. I also supply an alternate (incomplete) implementation of transaction that solves 3 out of 4 problems, the 4th turning out to be real hard to solve.

The code also seems to run fine under 2.0, although rspec expect {}.to raise_error seems to be broken for LocalJumpErrors.

It seems to me that you're explaining things to me that my alternate implementation just avoids doing entirely. So I am not entering discussion about closure behaviour. I am merely trying to make dm-transactions better.