New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocalJumpError: unexpected break in spec/ruby/language/break_spec.rb #4577

Closed
eregon opened this Issue Apr 28, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@eregon
Member

eregon commented Apr 28, 2017

jruby 9.1.9.0-SNAPSHOT (2.3.3) 2017-04-28 038f790 OpenJDK 64-Bit Server VM 25.121-b14 on 1.8.0_121-b14 +jit [linux-x86_64]

Which can be reproduced with:

 bin/jruby spec/mspec/bin/mspec run -Gfails -Gcritical -Gslow spec/ruby/language/break_spec.rb

This specific spec is currently excluded in jruby.2.3.mspec so that's why it got unnoticed.
There are only 3 ruby_exe calls in that spec so once this is fixed I would advise to include it on CI.

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jun 23, 2017

Member

@headius @enebo Could you have a look at this?

Member

eregon commented Jun 23, 2017

@headius @enebo Could you have a look at this?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 23, 2017

Member

Would be nice to get this fixed and the spec running.

Member

headius commented Jun 23, 2017

Would be nice to get this fixed and the spec running.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 23, 2017

Member

So this is another case where we are propagating the break out like we should, but we don't produce a rescuable LJE until it's too late. So there's some missing logic at the closure boundary (a proc in this case) to know that the break has no target and the LJE needs to start propagating earlier.

Member

headius commented Jun 23, 2017

So this is another case where we are propagating the break out like we should, but we don't produce a rescuable LJE until it's too late. So there's some missing logic at the closure boundary (a proc in this case) to know that the break has no target and the LJE needs to start propagating earlier.

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jun 26, 2017

Member

The full backtrace is

1)
An exception occurred during: loading /home/eregon/code/jruby/spec/ruby/language/break_spec.rb
The break statement in a captured block when the invocation of the scope creating the block is still active raises a LocalJumpError when invoking the block from the scope creating the block ERROR
LocalJumpError: unexpected break
org/jruby/RubyKernel.java:979:in `load'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:1:in `block in (root)'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:81:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:98:in `block in protect'
org/jruby/RubyArray.java:1734:in `each'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:81:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:72:in `each_file'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:77:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:48:in `process'
/home/eregon/code/jruby/spec/mspec/lib/mspec/commands/mspec-run.rb:83:in `run'
spec/mspec/bin/mspec-run:7:in `<main>'

So it seems the stack is unwound until the load call, which is way too far.
It should stop at the rescue inside:

lambda { @program.break_in_method }.should raise_error(LocalJumpError)
Member

eregon commented Jun 26, 2017

The full backtrace is

1)
An exception occurred during: loading /home/eregon/code/jruby/spec/ruby/language/break_spec.rb
The break statement in a captured block when the invocation of the scope creating the block is still active raises a LocalJumpError when invoking the block from the scope creating the block ERROR
LocalJumpError: unexpected break
org/jruby/RubyKernel.java:979:in `load'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:1:in `block in (root)'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:81:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:98:in `block in protect'
org/jruby/RubyArray.java:1734:in `each'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:81:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:72:in `each_file'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:77:in `files'
/home/eregon/code/jruby/spec/mspec/lib/mspec/runner/mspec.rb:48:in `process'
/home/eregon/code/jruby/spec/mspec/lib/mspec/commands/mspec-run.rb:83:in `run'
spec/mspec/bin/mspec-run:7:in `<main>'

So it seems the stack is unwound until the load call, which is way too far.
It should stop at the rescue inside:

lambda { @program.break_in_method }.should raise_error(LocalJumpError)
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 26, 2017

Member

Yes, it doesn't stop at the rescue for the reason I mentioned above. We need to raise the LJE immediately when we can determine the break is not valid, so it can be rescued.

A hacky alternative I've considered is to make LJE and superclasses match our non-local flow exceptions, but it would be better to just raise the real one.

Member

headius commented Jun 26, 2017

Yes, it doesn't stop at the rescue for the reason I mentioned above. We need to raise the LJE immediately when we can determine the break is not valid, so it can be rescued.

A hacky alternative I've considered is to make LJE and superclasses match our non-local flow exceptions, but it would be better to just raise the real one.

@headius headius added this to the JRuby 9.2.0.0 milestone Jun 26, 2017

@headius headius closed this in 203daad Jun 28, 2017

eregon added a commit that referenced this issue Jun 28, 2017

headius added a commit that referenced this issue Oct 10, 2017

Mark blocks as escaped and raise LJE for break when appropriate.
Fixes #4686.
Fixes #4577.

The logic here adds finally wrappers around all call paths that
receive a block. When the call exits, the block is marked
"escaped" since it no longer has a method activation to go with
it. This indicates that non-local flow, like break, should
immediately trigger a LocalJumpError.

This passes specs but has not been tested extensively with other
types of call forms that receive blocks.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 10, 2017

Member

Reopening to backport to 9.1.14.

Member

headius commented Oct 10, 2017

Reopening to backport to 9.1.14.

@headius headius reopened this Oct 10, 2017

@headius headius closed this Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment