Skip to content
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

$! does not get cleared under certain circumstances #2910

Closed
mkristian opened this issue May 5, 2015 · 14 comments
Closed

$! does not get cleared under certain circumstances #2910

mkristian opened this issue May 5, 2015 · 14 comments

Comments

@mkristian
Copy link
Member

$ gem install leafy-metrics
Fetching: leafy-metrics-0.4.0-java.gem (100%)
  jar dependencies for leafy-metrics-0.4.0-java.gemspec . . .
      io.dropwizard.metrics:metrics-core:3.1.0
      io.dropwizard.metrics:metrics-graphite:3.1.0
      io.dropwizard.metrics:metrics-jvm:3.1.0
      org.slf4j:slf4j-simple:1.7.7
Successfully installed leafy-metrics-0.4.0-java
ERROR:  While executing gem ... (Psych::SyntaxError)
    (<unknown>): mapping values are not allowed here at line 13 column 151
@mkristian
Copy link
Member Author

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.

@mkristian mkristian reopened this May 6, 2015
@mkristian mkristian changed the title can not install leafy-metrics $! does not get cleared uncertain circumstances May 6, 2015
@headius
Copy link
Member

headius commented May 7, 2015

Great find.

@mkristian mkristian changed the title $! does not get cleared uncertain circumstances $! does not get cleared under certain circumstances May 7, 2015
@subbuss
Copy link
Contributor

subbuss commented May 8, 2015

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 $!

@headius
Copy link
Member

headius commented May 8, 2015

Looking into it now.

@headius
Copy link
Member

headius commented May 8, 2015

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?

@subbuss
Copy link
Contributor

subbuss commented May 8, 2015

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.

@headius
Copy link
Member

headius commented May 8, 2015

(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).

@headius
Copy link
Member

headius commented May 8, 2015

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 ! e n s u r e t o j u s t t h e r e s c u e s , b u t t h e n I r e a l i z e d t h e r e m a y b e n o n e r r o r s t a c k u n r o l l s i n t h e e n s u r e t h a t p r e v e n t ! from being reset there.

I think this is the best approach. We can probably remove some ! h a n d l i n g e l s e w h e r e i n I R b u t I m n o t s u r e w h e r e t h a t a l l i s a n d w h a t s s a f e t o r e m o v e ( f o r e x a m p l e , l o c a l b r e a k / n e x t s t i l l n e e d ! logic because they are neither fall-through nor exceptional exit from the construct).

@headius
Copy link
Member

headius commented May 9, 2015

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

@headius headius closed this as completed May 9, 2015
@headius headius added this to the JRuby 9.0.0.0.rc1 milestone May 9, 2015
@headius
Copy link
Member

headius commented May 9, 2015

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

@headius headius reopened this May 9, 2015
@headius headius added ir and removed stdlib labels May 9, 2015
subbuss added a commit that referenced this issue May 9, 2015
* This fixes the snippets in #2910.
* More needs to be done to clean this up and get rid of other
  save-restore code for $! elsewhere.
* Committing this to check if it successfully passes travis tests.
@subbuss
Copy link
Contributor

subbuss commented May 9, 2015

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.

@subbuss
Copy link
Contributor

subbuss commented May 9, 2015

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

@subbuss
Copy link
Contributor

subbuss commented May 9, 2015

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.

@subbuss
Copy link
Contributor

subbuss commented May 9, 2015

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.

@subbuss subbuss closed this as completed May 16, 2015
subbuss added a commit that referenced this issue Aug 19, 2015
Fix for #2910 added a try-catch-finally around bare rescue nodes
to make sure $! was restored on non-local exit paths. However,
we need to make sure that the restore didn't happen on exception
exit paths (for ex. when the exception was not rescued).
Hence #3237.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants