JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-4262

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

          Activity

          Hide
          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.
          Hide
          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.
          Hide
          Vladimir Sizikov added a comment -

          Linking to JRUBY-2760.

          Show
          Vladimir Sizikov added a comment - Linking to JRUBY-2760 .
          Hide
          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
          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.
          Hide
          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.
          Hide
          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.

            People

            • Assignee:
              Charles Oliver Nutter
              Reporter:
              Vladimir Sizikov
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: