-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Comments
I reduced the actually problem to
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 Do we have a generic way of adding |
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:
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...
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 I think this is the best approach. We can probably remove some |
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.
|
* 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.
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. |
The text was updated successfully, but these errors were encountered: