I reduced the actually problem to

require 'fileutils'
def gem_specification
  begin
    raise
  rescue Exception
    FileUtils.cd( '.' ) do
      return
    end
  end
end
gem_specification

p $!

jruby-1.7.x prints 'nil' and jruby-9k prints 'RuntimeError'

i.e. somehow $! does not get cleared.

Great find.

This is probably a IR generation issue wrt clearing $! before non-local returns .. See simpler test case below.

def foo; yield; end

def bar
  begin
    raise
  rescue Exception
    foo { return }
  end
end

bar

p $!

Looking into it now.

Ok, so there may be a couple ways to do this and I'm not sure what's best. I think our simplest choice would be to add logic to rescue compilation to always ensure $! gets reset on the way out. This would require an additional layer of finally logic to surround all rescued blocks.

Do we have a generic way of adding finally logic around a region of code?

I spent some time y'day night staring at this, but, the additional layer of finally logic seems excessive. I'll be online on IRC in about 15-30 mins and let us chat a bit more there.

(Comment added to wrong bug before @subbuss comment above)

Here's what I came up with: https://gist.github.com/headius/94511fa81a28733787f0

The logic in resetLastErrorPreamble/Postamble is duplicated from buildEnsureNode. Both buildEnsureNode and buildRescueNode (the top-level versions) put this pre/post logic around the entire begin+rescue+etc.

There are a few issues, obviously:

• @subbuss had been working to make $! resetting happen exactly where it needed to, but in this case that would have required doing it for every RescueBody. This returns us to a single big try/finally around the whole of rescue/ensure logic.
• There are places where we restore $! that are obviated by this change.
• It adds another BB and another exception region.

This is deeper into IR than I've gone before, so I'd like some comments and review on this. I will push a PR to see if it passes (it passed what I tried locally and fixes the original issue).

I need to write down some notes to remember things...

• Unlike in MRI, we do not normally have $! set while an exception is in flight; rather, we set it whenever we enter rescues.
• My logic resets $! for all exits out of a begin/rescue/ensure/else construct, preserving the above requirement.
• Within a rescue, $! is always set to current exception.
• Within a begin or an else, $! is always set to previous exception.
• Within an ensure, $! is previous exception for normal exit and current exception for exceptional exit.

In order to preserve all these states, we need an ensure wrapping the entirety of begin/rescue/else/ensure. This allows $! to remain untouched through the entire construct and guarantees it will be cleared after we leave the construct.

I had thought I could localize the $!ensuretojusttherescues,butthenIrealizedtheremaybenon-errorstackunrollsintheensurethatprevent$! from being reset there.

I think this is the best approach. We can probably remove some $!handlingelsewhereinIRbut{I}^{\prime }mnotsurewherethatallisandwha{t}^{\prime }ssafetoremove(forexample,localbreak/nextstillneed$! logic because they are neither fall-through nor exceptional exit from the construct).

Going to call this fixed with my patch, but I'd love to hear about any improvements I can make.

Blast, this didn't work right. The following tests fail with my change...but ONLY these two tests, which is truly baffling.

     [exec]   1) Failure:
     [exec] CGIMultipartTest#test_cgi_multipart_maxmultipartcount [/home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:278]:
     [exec] [StandardError] exception expected, not.
     [exec] Class: <TypeError>
     [exec] Message: <"assigning non-exception to $!">
     [exec] ---Backtrace---
     [exec] /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:598:in `create_body'
     [exec] /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:490:in `read_multipart'
     [exec] /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:648:in `initialize_query'
     [exec] /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:851:in `initialize'
     [exec] /home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:155:in `_test_multipart'
     [exec] /home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:279:in `block in test_cgi_multipart_maxmultipartcount'
     [exec] /home/travis/build/jruby/jruby/test/mri/lib/test/unit/assertions.rb:73:in `assert_raise'
     [exec] /home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:278:in `test_cgi_multipart_maxmultipartcount'
     [exec] /home/travis/build/jruby/jruby/test/mri/lib/test/unit.rb:888:in `run_test'
     [exec] ---------------
     [exec] 
     [exec]   2) Error:
     [exec] CGIMultipartTest#test_cgi_multipart_tempfile:
     [exec] TypeError: assigning non-exception to $!
     [exec]     /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:598:in `create_body'
     [exec]     /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:490:in `read_multipart'
     [exec]     /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:648:in `initialize_query'
     [exec]     /home/travis/build/jruby/jruby/lib/ruby/stdlib/cgi/core.rb:851:in `initialize'
     [exec]     /home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:155:in `_test_multipart'
     [exec]     /home/travis/build/jruby/jruby/test/mri/cgi/test_cgi_multipart.rb:224:in `test_cgi_multipart_tempfile'
     [exec]     /home/travis/build/jruby/jruby/test/mri/lib/test/unit.rb:888:in `run_test'

See https://travis-ci.org/jruby/jruby/jobs/61844969

I pushed a simpler fix. So far, I was using common rescue-internal code for handling ensure nodes (since they shared rescue building code).

Now that we need to add dummy ensure code for rescues, I flipped the situation around and treated rescue nodes like ensure nodes. This eliminates special cases and makes sure everything works consistently.

If travis passes, there is a bunch of cleanup to do here to eliminate $! save/restore from a bunch of other places.

So greens are build, so I think we are good to go. I have one more minor cleanup that I'll commit .. but, with that commit, the net change to fix this is: https://gist.github.com/subbuss/402021e59df2985be60a

I'll now start cleanup to remove duplicate $! save/restore code.

Done. Pushed additional cleanup. Hopefully travis will be happy. We should look at fixing retries so we can finish off the cleanup. The next step is to look at the runtime lib code to make sure all try-catch for $! save-restore is replaced with try-catch-finally.