JRuby

Weird Proc test makes JRuby to skip the rest of the tests completely

Details

  • Number of attachments :
    0

Description

This is a blocker.

Essentially, in both, 1.8 and 1.9 mode, once JRuby executes
core/proc/new_spec.rb, it doesn't execute anything alse AT ALL, just
pretends

Take a look:

mspec -t j core/proc/new_spec.rb core

Here, I execute that evil proc/new_spec first, and then the entire
core specs, and the results are:

1427 files, 1 example, 2 expectations.

TWO expectations only. That explains also why you don't see the
String errors when running the whole core set, because proc/new is
before the string, and essentially the string specs are not executed
at all!!!

And the spec in question that
breaks JRuby is:

 # This raises a ThreadError on 1.8 HEAD. Reported as bug #1707
 it "raises a LocalJumpError when context of the block no longer exists" do
   def some_method
     Proc.new { return }
   end
   res = some_method()

   # Using raise_error here causes 1.9 to hang, so we roll our own
   # begin/rescue block to verify that the exception is raised.

   exception = nil

   begin
     res.call  #### <-------------------- *** Commenting out this line would allow to run other tests ***********
   rescue LocalJumpError => e
     exception = e
   end

   e.should be_an_instance_of(LocalJumpError)
 end

Btw, MRI works just fine here, so it does look like JRuby's issue.

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. JRUBY-2760 break/return does not become a LocalJumpError when it should

  • Major - Major loss of function.
  • Resolved - A resolution has been taken, and it is awaiting verification by reporter. From here issues are either reopened, or are closed.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Vladimir Sizikov added a comment -

See the mailing list discussion as well.

Show
Vladimir Sizikov added a comment - See the mailing list discussion as well.
Vladimir Sizikov made changes -
Field Original Value New Value
Description This is a blocker.

Essentially, in *both*, 1.8 and 1.9 mode, once JRuby executes
core/proc/new_spec.rb, it doesn't execute anything alse AT ALL, just
pretends :)

Take a look:

mspec -t j core/proc/new_spec.rb core

Here, I execute that evil proc/new_spec first, and then the entire
core specs, and the results are:

1427 files, 1 example, 2 expectations.

*TWO* expectations only. That explains also why you don't see the
String errors when running the whole core set, because proc/new is
before the string, and essentially the string specs are not executed
at all!!!

And the spec in question that
breaks JRuby is:

{code}
 # This raises a ThreadError on 1.8 HEAD. Reported as bug #1707
 it "raises a LocalJumpError when context of the block no longer exists" do
   def some_method
     Proc.new { return }
   end
   res = some_method()

   # Using raise_error here causes 1.9 to hang, so we roll our own
   # begin/rescue block to verify that the exception is raised.

   exception = nil

   begin
     res.call #### <-------------------- *** Commenting out this
line would allow to run other tests ***********
   rescue LocalJumpError => e
     exception = e
   end

   e.should be_an_instance_of(LocalJumpError)
 end
{code}

Btw, MRI works just fine here, so it does look like JRuby's issue. 		This is a blocker.

Essentially, in *both*, 1.8 and 1.9 mode, once JRuby executes
core/proc/new_spec.rb, it doesn't execute anything alse AT ALL, just
pretends :)

Take a look:

mspec -t j core/proc/new_spec.rb core

Here, I execute that evil proc/new_spec first, and then the entire
core specs, and the results are:

1427 files, 1 example, 2 expectations.

*TWO* expectations only. That explains also why you don't see the
String errors when running the whole core set, because proc/new is
before the string, and essentially the string specs are not executed
at all!!!

And the spec in question that
breaks JRuby is:

{code}
 # This raises a ThreadError on 1.8 HEAD. Reported as bug #1707
 it "raises a LocalJumpError when context of the block no longer exists" do
   def some_method
     Proc.new { return }
   end
   res = some_method()

   # Using raise_error here causes 1.9 to hang, so we roll our own
   # begin/rescue block to verify that the exception is raised.

   exception = nil

   begin
     res.call #### <-------------------- *** Commenting out this line would allow to run other tests ***********
   rescue LocalJumpError => e
     exception = e
   end

   e.should be_an_instance_of(LocalJumpError)
 end
{code}

Btw, MRI works just fine here, so it does look like JRuby's issue.
Vladimir Sizikov made changes -
Summary Weird Proc tests makes JRuby to skip the rest of the tests completely Weird Proc test makes JRuby to skip the rest of the tests completely
Hide
Permalink
Yoko Harada added a comment -

This looks similar issue to JRUBY-2760, doesn't it? When res.call is commented out, no error is raised. This ends up in ... rescue block isn't used, so just works fine.

Show
Yoko Harada added a comment - This looks similar issue to JRUBY-2760 , doesn't it? When res.call is commented out, no error is raised. This ends up in ... rescue block isn't used, so just works fine.
Vladimir Sizikov made changes -
Link This issue is related to JRUBY-2760 [ JRUBY-2760 ]
Hide
Permalink
Vladimir Sizikov added a comment -

Linking to JRUBY-2760.

Show
Vladimir Sizikov added a comment - Linking to JRUBY-2760 .
Hide
Permalink
Charles Oliver Nutter added a comment -

This can be trivially reproduced like this:

jruby -e "def some_method; Proc.new

{ return }

; end; begin; some_method.call; rescue LocalJumpError => e; puts e; end; puts 'here'"

I imagine the ReturnJump is just not being turned into a LocalJumpError, and then happily caught by Main so the process just exits quietly.

Show
Charles Oliver Nutter added a comment - This can be trivially reproduced like this: jruby -e "def some_method; Proc.new { return } ; end; begin; some_method.call; rescue LocalJumpError => e; puts e; end; puts 'here'" I imagine the ReturnJump is just not being turned into a LocalJumpError, and then happily caught by Main so the process just exits quietly.
Hide
Permalink
Charles Oliver Nutter added a comment -

I fixed this in a somewhat inefficient way in cbe7b2b.

The fix was as follows:

  • Check if the block has "escaped", which means it was captured and the call it was passed to is no longer active. If it has escaped...
  • Dig through the current frame stack to see if it contains a frame containing the JumpTarget used for the return. If no such frame is found, a LocalJumpError is thrown.

In MRI (at least, in the past), this searching was done for any non-local return, and the LocalJumpError was raised immediately. In order to reduce the overhead, we are only doing it for blocks wrapped in a Proc, which should be fairly rare anyway.

Show
Charles Oliver Nutter added a comment - I fixed this in a somewhat inefficient way in cbe7b2b. The fix was as follows: Check if the block has "escaped", which means it was captured and the call it was passed to is no longer active. If it has escaped... Dig through the current frame stack to see if it contains a frame containing the JumpTarget used for the return. If no such frame is found, a LocalJumpError is thrown. In MRI (at least, in the past), this searching was done for any non-local return, and the LocalJumpError was raised immediately. In order to reduce the overhead, we are only doing it for blocks wrapped in a Proc, which should be fairly rare anyway.
Charles Oliver Nutter made changes -
Assignee Charles Oliver Nutter [ headius ]
Status Open [ 1 ] Resolved [ 5 ]
Resolution Fixed [ 1 ]
Hide
Permalink
Charles Oliver Nutter added a comment -

Bleh, I'm not sure what went wrong, but this hasn't actually fixed the issue.

Show
Charles Oliver Nutter added a comment - Bleh, I'm not sure what went wrong, but this hasn't actually fixed the issue.
Charles Oliver Nutter made changes -
Resolution Fixed [ 1 ]
Status Resolved [ 5 ] Reopened [ 4 ]
Hide
Permalink
Charles Oliver Nutter added a comment -

Ok, when the simple fix doesn't work, do the complicated fix.

In 3587b55, I have replaced all uses of JumpTarget with an int, based off the "calls" int in ThreadContext. The idea is that for non-local jump purposes we want to be able to uniquely identify a given call, so we can both search for that call (to see if it's still active) and compare blocks against a given call without collision. Since we are already spinning a per-thread int for periodic thread event polling, it made sense to reuse that same int.

The change replaces all uses of JumpTarget with this new int value, and frame pushes now set a new int jumpTarget every time; i.e. each activation gets a new id.

This fixes the original case and does not break the define_method+proc

{return}

case I broke.

Along with this change, I also moved the catch/throw logic to using the same behavior as one-shot continuations, since they're functionally identical.

Show
Charles Oliver Nutter added a comment - Ok, when the simple fix doesn't work, do the complicated fix. In 3587b55, I have replaced all uses of JumpTarget with an int, based off the "calls" int in ThreadContext. The idea is that for non-local jump purposes we want to be able to uniquely identify a given call , so we can both search for that call (to see if it's still active) and compare blocks against a given call without collision. Since we are already spinning a per-thread int for periodic thread event polling, it made sense to reuse that same int. The change replaces all uses of JumpTarget with this new int value, and frame pushes now set a new int jumpTarget every time; i.e. each activation gets a new id. This fixes the original case and does not break the define_method+proc {return} case I broke. Along with this change, I also moved the catch/throw logic to using the same behavior as one-shot continuations, since they're functionally identical.
Charles Oliver Nutter made changes -
Status Reopened [ 4 ] Resolved [ 5 ]
Resolution Fixed [ 1 ]
Charles Oliver Nutter made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

  • Assignee:
    Charles Oliver Nutter
    Reporter:
    Vladimir Sizikov
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: