LocalJumpError: unexpected return #12981

Closed
chancancode opened this Issue Nov 21, 2013 · 25 comments

Projects

None yet

8 participants

@chancancode
Member

TL;DR

# This doesn't work anymore (can't use return in callback blocks)

class Model < AR::Base
  before_save do
    return false if some_condition

    # ...more stuff here
  end
end

# Fix 1

class Model < AR::Base
  before_save do
    if some_condition
      false
    else
      # ...more stuff here
    end
  end
end

# Fix 2

class Model < AR::Base
  before_save(&->{
    return false if some_condition

    # ...more stuff here
  })
end

# Fix 3

class Model < AR::Base
  before_save :my_before_save

  def my_before_save
    return false if some_condition

    # ...more stuff here
  end
end

Moar

This is affecting the master branch only.

I am upgrading Discourse to run on master so I can run some benchmarks on it. One error that I encountered when upgrading is this:

     LocalJumpError:
       unexpected return
     # ./app/models/topic.rb:156:in `block in <class:Topic>'

...which is caused by code like this:

  after_create do
    return if skip_callbacks

    changed_to_category(category)
    if archetype == Archetype.private_message
      DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
    else
      DraftSequence.next!(user, Draft::NEW_TOPIC)
    end
  end

...which I fixed it by changing it to...

  after_create do
    unless skip_callbacks
      changed_to_category(category)
      if archetype == Archetype.private_message
        DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
      else
        DraftSequence.next!(user, Draft::NEW_TOPIC)
      end
    end
  end

Is the return early pattern supported? If so this seems to be a breaking change.

From my initial investigation it looks like this is caused by the changes made to how ActiveSupport callbacks work.

cc @tenderlove @SamSaffron

@tenderlove
Member

Yes, we don't 'eval' the code in to methods anymore, and returning from a lambda isn't a thing you can do.

Not sure what to do. I don't want to bring back the eval hell.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Nov 21, 2013, at 4:58 PM, Godfrey Chan notifications@github.com wrote:

This is affecting the master branch only.

I am upgrading Discourse to run on master so I can run some benchmarks on it. One error that I encountered when upgrading is this:

 LocalJumpError:
   unexpected return
 # ./app/models/topic.rb:156:in `block in <class:Topic>'

...which is caused by code like this:

after_create do
return if skip_callbacks

changed_to_category(category)
if archetype == Archetype.private_message
  DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
else
  DraftSequence.next!(user, Draft::NEW_TOPIC)
end

end
...which I fixed it by changing it to...

after_create do
unless skip_callbacks
changed_to_category(category)
if archetype == Archetype.private_message
DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
else
DraftSequence.next!(user, Draft::NEW_TOPIC)
end
end
end
Is the return early pattern supported? If so this seems to be a breaking change.

From my initial investigation it looks like this is caused by the changes made to how ActiveSupport callbacks work.

cc @tenderlove @SamSaffron


Reply to this email directly or view it on GitHub.

@chancancode
Member

Maybe it just need to document it and add it to the release notes / blog post / upgrade guide then?

@tenderlove
Member

Actually, we control the place where the lambda is called. This is
extremely horrible, but I think we could catch the exception and issue a
warning.

On Thursday, November 21, 2013, Godfrey Chan wrote:

This is affecting the master branch only.

I am upgrading Discourse https://github.com/discourse/discourse to run
on master so I can run some benchmarks on it. One error that I encountered
when upgrading is this:

 LocalJumpError:
   unexpected return
 # ./app/models/topic.rb:156:in `block in <class:Topic>'

...which is caused by code like this:

after_create do
return if skip_callbacks

changed_to_category(category)
if archetype == Archetype.private_message
  DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
else
  DraftSequence.next!(user, Draft::NEW_TOPIC)
end

end

...which I fixed it by changing it to...

after_create do
unless skip_callbacks
changed_to_category(category)
if archetype == Archetype.private_message
DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
else
DraftSequence.next!(user, Draft::NEW_TOPIC)
end
end
end

Is the return early pattern supported? If so this seems to be a breaking
change.

From my initial investigation it looks like this is caused by the changes
made to how ActiveSupport callbacks work.

cc @tenderlove https://github.com/tenderlove @SamSaffronhttps://github.com/SamSaffron


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/12981
.

Aaron Patterson
http://tenderlovemaking.com/

@chancancode
Member

Hmm, but you won't have access to the "return value" in that case, correct? I haven't seen it so far, but I wouldn't be too surprised if people use this pattern to halt the callbacks chain by returning false early (vs return true or just return). But either way, even if we can't do the "correct" thing automagically, an informative warning still sounds like a good idea?

@al2o3cr
Contributor
al2o3cr commented Nov 24, 2013

FWIW, the best advice is to replace return with break - you can even specify a return value:

breaking_proc = lambda { break 'foo'; }
breaking_proc.call # => 'foo'
@SamSaffron
Contributor

@al2o3cr great point!

@melekes
Contributor
melekes commented Nov 28, 2013

@al2o3cr 👍

@chancancode
Member

@tenderlove Can confirm that 1. it's not possible to make return work anymore 2. break is the correct substitute?

If both are true, I'll close this and add a test case to ensure break works going forward, and mention this in the upgrade docs.

@rafaelfranca
Member

@chancancode please go ahead.

@chancancode
Member

So, I just tried adding some tests for this, by they all seems to be passing for me here.

I also tried this:

~% irb
>> p = ->{ return 1 }
=> #<Proc:0x007ff20bd773c8@(irb):1 (lambda)>
>> p.call
=> 1
>> l = lambda { return 1 }
=> #<Proc:0x007ff20bd5ea58@(irb):3 (lambda)>
>> l.call
=> 1

Related stackoverflow question.

Since this is working for AS::Callbacks, I need to look into whether this is specific to AR callbacks or not.

@chancancode chancancode referenced this issue in discourse/discourse Dec 8, 2013
Merged

Make Discourse work on Rails master (4.1) #1709

8 of 8 tasks complete
@chancancode
Member

Hey guys, I just added these failing tests in ActiveRecord, turns out neither works.

  1) Error:
CallbacksTest#test_before_save_return_early_using_break:
LocalJumpError: break from proc-closure
    test/cases/callbacks_test.rb:190:in `block in <class:CallbackReturnDeveloper>'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:438:in `instance_exec'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:438:in `block in make_lambda'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:160:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:160:in `block in halting'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/callbacks.rb:302:in `create_or_update'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/persistence.rb:103:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/validations.rb:51:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/attribute_methods/dirty.rb:21:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:272:in `block (2 levels) in save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:329:in `block in with_transaction_returning_status'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:200:in `block in transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:208:in `within_new_transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:200:in `transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:211:in `transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:326:in `with_transaction_returning_status'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:272:in `block in save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:283:in `rollback_active_record_state!'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:271:in `save'
    test/cases/callbacks_test.rb:461:in `test_before_save_return_early_using_break'


  2) Error:
CallbacksTest#test_before_save_return_early_using_return:
LocalJumpError: unexpected return
    test/cases/callbacks_test.rb:189:in `block in <class:CallbackReturnDeveloper>'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:438:in `instance_exec'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:438:in `block in make_lambda'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:160:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:160:in `block in halting'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/callbacks.rb:302:in `create_or_update'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/persistence.rb:103:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/validations.rb:51:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/attribute_methods/dirty.rb:21:in `save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:272:in `block (2 levels) in save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:329:in `block in with_transaction_returning_status'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:200:in `block in transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:208:in `within_new_transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:200:in `transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:211:in `transaction'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:326:in `with_transaction_returning_status'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:272:in `block in save'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:283:in `rollback_active_record_state!'
    /Users/godfrey/Projects/oss/rails-chancancode/activerecord/lib/active_record/transactions.rb:271:in `save'
    test/cases/callbacks_test.rb:454:in `test_before_save_return_early_using_return'

I'm afraid this might be a real bug that needs to be fixed. Since the ActiveSupport tests work just fine, I think there might be ways to make this work, investigating...

@chancancode chancancode added a commit to chancancode/rails that referenced this issue Dec 8, 2013
@chancancode chancancode Added failing tests for #12981 97e3d04
@chancancode
Member

Added the same failing tests in ActiveModel

  1) Error:
CallbacksTest#test_chain_is_halted_if_a_callback_returns_false_with_break:
LocalJumpError: break from proc-closure
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:28:in `block in <class:ModelCallbacks>'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:440:in `instance_exec'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:440:in `block in make_lambda'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:221:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:221:in `block in halting_and_conditional'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:43:in `create'
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:79:in `block in <class:CallbacksTest>'


  2) Error:
CallbacksTest#test_chain_is_halted_if_a_callback_returns_false_with_return:
LocalJumpError: unexpected return
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:27:in `block in <class:ModelCallbacks>'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:440:in `instance_exec'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:440:in `block in make_lambda'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:221:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:221:in `block in halting_and_conditional'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /Users/godfrey/Projects/oss/rails-chancancode/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:43:in `create'
    /Users/godfrey/Projects/oss/rails-chancancode/activemodel/test/cases/callbacks_test.rb:72:in `block in <class:CallbacksTest>'
@chancancode
Member

Actually, it seems like the problem is in fact in the ActiveSupport rewrite, I think my tests are just testing the wrong thing:

>> def call(&block); block.call; end
=> nil
>> a(&->{ return 1 })
=> 1
>> a(&->{ break 1 })
=> 1
>> a(&->{ 1 })
=> 1
>> a { return 1 }
LocalJumpError: unexpected return
    from (irb):12:in `block in irb_binding'
    from (irb):2:in `call'
    from (irb):2:in `a'
    from (irb):12
    from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'
>> a { break 1 }
=> 1
>> a { 1 }
=> 1
@chancancode
Member

Still not sure why break works here but not in my tests. It's getting late and I need to move on to some other things for tomorrow, so if anyone wants to pick it up from here please go ahead. Otherwise I can probably return to this early next week.

@chancancode
Member

Just confirmed break doesn't work when upgrading Discourse, had lots of pain trying to convert these type of inline callbacks. Switching to block syntax or converting them into method seems equally painful/ugly.

@chancancode
Member

Did some more research & experiments, seems like this might very well be impossible to fix unless we bring back the "'eval' into methods" trick, but it looks like that's a no-go also?

@chancancode
Member

(The change was introduced in 2b1500d#diff-1ecd313ff0ab827af30014553cf8918dL354)

@fxn
Member
fxn commented Dec 10, 2013

Let me recap for people following the thread how does return work: return returns from the enclosing method if there is one:

def x
  yield
  1
end

def y
  x { return 0 }
  2
end

p y # prints 0

If there is no enclosing method, then you get LocalJumpError:

class C
  return # raises LocalJumpError
end

Note that the enclosing method is not the one invoking the block. That return in the first example does not make the block return to x, it directly makes y return. This is thus lexical most of the time, you look at the source code and see where are we exiting from. The invoking method has no control over that unless it creates a wrapping method:

def foo(&block)
  self.class.send(:define_method, :bar, &block)
  bar
end

p foo { return 1 } # prints 1

An exception to that rule are lambdas, return within a lambda returns from the lambda:

def foo
  a = -> { return 0 } # returns from a, not from foo
  p a.call
  p :fine
end

foo # prints 0 and :fine

A top-level break in a lambda acts as a return from the lambda:

def foo
  a = -> { break 1 }
  a.call
end

p foo # prints 1
@mitio
Contributor
mitio commented Dec 10, 2013

I just want to point out that you can convert a block object into a lambda and allow it to contain a return, like so:

def return_in_blocks(&block)
  puts 'Before calling the block'
  lambda(&block).call
  puts 'After calling the block'
end

return_in_blocks { puts 'Hi from the block! I will now return.'; return }

This code will outpuit the following:

Before calling the block
Hi from the block! I will now return.
After calling the block

It should also be noted that converting a block to a lambda will make it stricter in regard to the number of arguments that the block is called with, if any.

@chancancode
Member

@mitio, have you tried that? I'm pretty sure that doesn't work.

% irb
>> def to_lambda(&block)
>>   puts block.lambda?
>>   puts lambda(&block).lambda?
>>   lambda(&block).call
>> end
=> nil
>> to_lambda { return 1 }
false
false
LocalJumpError: unexpected return
    from (irb):6:in `block in irb_binding'
    from (irb):4:in `call'
    from (irb):4:in `to_lambda'
    from (irb):6
    from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'

The documentation specifically points out that passing a lambda block to proc() or vice versa, the lambda (proc) property will be preserved. There is also a ticket on Ruby redmine for this.

@mitio
Contributor
mitio commented Dec 10, 2013

@chancancode Interesting. It works on Ruby 1.8.7. I didn't test it on newer versions since I didn't expect it won't work. Good catch, thanks for the analysis and sorry for the misleading information. No solution awaits in this path, apparently.

I guess a way to make return work could be to use some kind of a define_method hack on an anonymous class or something like that. I doubt it would have good portablility across different implementations, though.

@chancancode
Member

@mitio No worries! Definitely appreciates more eyes on this one. I tried the define_method hack too, but that gives you a bound method so you can't access the instance variables/methods from the block =/

I think it was decided that this "feature" was never intentionally supported and the fact that it was working before was due to the specific way this is implemented (the blocks are being defined as methods on the class), so I'm gonna submit a PR to mention this in the upgrade guides instead. I'll summarize everyone's findings over there.

@fxn
Member
fxn commented Dec 10, 2013

👍, returning from a block like that does not seem to be something you're supposed to do.

@chancancode
Member

For everyone that's following this, I have submitted #13271 with a detailed analysis of what's going on here

@chancancode chancancode added a commit to chancancode/rails that referenced this issue Dec 11, 2013
@chancancode chancancode Warn about using `return` inside inline callback blocks [ci skip]
Closes #12981
68abbac
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui kamui Swap return for break in AR callback for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
17b976a
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
8daecef
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
db23b97
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
c189d7e
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
c58b5f9
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
ae156cc
@kamui kamui referenced this issue in inossidabile/protector Dec 30, 2013
Merged

Define AR callback as method for Rails 4.1 compatibility #38

@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
510ebf4
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
edc1e4d
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
0bfe4df
@kamui kamui added a commit to kamui/protector that referenced this issue Dec 30, 2013
@kamui @kamui kamui + kamui define AR callback as method for Rails 4.1 compatibility
rails/rails#12981

This was causing me issues when trying to destroy an instance of a model with a protect block defined.
5f2c8d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment