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

Catching RaiseException resolves the issue with Mirah macros. [JRUBY-6043] #72

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@abscondment

abscondment commented Sep 7, 2011

Catching RaiseException resolves the issue with Mirah macros. [JRUBY-6043]

Should also go into jruby-1_6.

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Sep 7, 2011

Contributor

Do we want to silently catch all RaiseExceptions, or catch it and then check to see whether it's one of the ones we need to catch and if not reraise? Just wondering what would happen if you had code like this:

class MyClassLoader < Java::SomeClassLoader
  SUFFIX_OF_WRONG_TYPE = 1
  def findClass name
    super(name + SUFFIX_OF_WRONG_TYPE)
  end
end

You'd want to get the error somehow. Otoh, I guess it'd blow up in other more interesting ways. @headius, thoughts?

Contributor

baroquebobcat commented Sep 7, 2011

Do we want to silently catch all RaiseExceptions, or catch it and then check to see whether it's one of the ones we need to catch and if not reraise? Just wondering what would happen if you had code like this:

class MyClassLoader < Java::SomeClassLoader
  SUFFIX_OF_WRONG_TYPE = 1
  def findClass name
    super(name + SUFFIX_OF_WRONG_TYPE)
  end
end

You'd want to get the error somehow. Otoh, I guess it'd blow up in other more interesting ways. @headius, thoughts?

@tobias

This comment has been minimized.

Show comment
Hide comment
@tobias

tobias Sep 12, 2011

Contributor

Maybe we should revert the catch block to blanket catch Exception, since it seems to break GAE as well, and possibly other things we have yet to see. I switched to a list of expected exception types because blanket exceptions disgust me a little, but we're wrapping an awful lot of code in that try block, so maybe a blanket exception makes sense. That, or an audit to see all of the possible "acceptable" exceptions that could be thrown, or a refactoring of the method to introduce finer grained try blocks.

Contributor

tobias commented Sep 12, 2011

Maybe we should revert the catch block to blanket catch Exception, since it seems to break GAE as well, and possibly other things we have yet to see. I switched to a list of expected exception types because blanket exceptions disgust me a little, but we're wrapping an awful lot of code in that try block, so maybe a blanket exception makes sense. That, or an audit to see all of the possible "acceptable" exceptions that could be thrown, or a refactoring of the method to introduce finer grained try blocks.

@BanzaiMan

This comment has been minimized.

Show comment
Hide comment
@BanzaiMan

BanzaiMan Oct 9, 2011

Member

Toby,

Can you put together a patch? While I think that a complete census of acceptable exceptions is preferable, I think that we need a reasonable fix for the short term.

Member

BanzaiMan commented Oct 9, 2011

Toby,

Can you put together a patch? While I think that a complete census of acceptable exceptions is preferable, I think that we need a reasonable fix for the short term.

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Oct 9, 2011

Contributor

What about instead of catching raise exception at the bottom, we wrap a try catch around the problematic loadClass that catches RaiseExceptions and reraises them as their java exceptions if they are java exceptions?

something like

        try {
        Class<?> companionClass = cl.loadClass(javaClass.getName() + "$");
        } catch (RaiseException e) {
        if (e.getException() instanceof NativeException) {
            throw ((NativeException) e).getCause(); // reraise native exceptions so we can ignore them
        }
        }
Contributor

baroquebobcat commented Oct 9, 2011

What about instead of catching raise exception at the bottom, we wrap a try catch around the problematic loadClass that catches RaiseExceptions and reraises them as their java exceptions if they are java exceptions?

something like

        try {
        Class<?> companionClass = cl.loadClass(javaClass.getName() + "$");
        } catch (RaiseException e) {
        if (e.getException() instanceof NativeException) {
            throw ((NativeException) e).getCause(); // reraise native exceptions so we can ignore them
        }
        }
@tobias

This comment has been minimized.

Show comment
Hide comment
@tobias

tobias Oct 10, 2011

Contributor

I submitted a pull request that restores blanket Exception catching: #89

@baroquebobcat - I'd rather go back to the blanket catch until a proper audit can be done to catch all of the potential exceptions.

Contributor

tobias commented Oct 10, 2011

I submitted a pull request that restores blanket Exception catching: #89

@baroquebobcat - I'd rather go back to the blanket catch until a proper audit can be done to catch all of the potential exceptions.

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Oct 11, 2011

Contributor

@tobias the particular case I've been having issues with is when you have a JRuby implemented classloader, where if the classloader raises one of the checked exceptions, it is wrapped with a RaiseException.

So I was thinking we could catch the RaiseException and unwrap it when it is one of the checked exceptions from a classloader. I'm not sure of the best way to do that however as I'm still pretty unfamiliar with JRuby's internals.

otoh, reverting back to the blanket catch will fix the issue I've been seeing, so that's fine with me. I just wanted to clarify what I meant.

Thanks for submitting a fix.

Contributor

baroquebobcat commented Oct 11, 2011

@tobias the particular case I've been having issues with is when you have a JRuby implemented classloader, where if the classloader raises one of the checked exceptions, it is wrapped with a RaiseException.

So I was thinking we could catch the RaiseException and unwrap it when it is one of the checked exceptions from a classloader. I'm not sure of the best way to do that however as I'm still pretty unfamiliar with JRuby's internals.

otoh, reverting back to the blanket catch will fix the issue I've been seeing, so that's fine with me. I just wanted to clarify what I meant.

Thanks for submitting a fix.

@tobias

This comment has been minimized.

Show comment
Hide comment
@tobias

tobias Oct 11, 2011

Contributor

@baroquebobcat I wanted to revert to the blanket exception, since other exceptions in addition to RaiseException were slipping through, and affecting other JRuby consumers (Google App Engine, for example). I wanted to get 1.6.5 back to a known working state before auditing for all possible exceptions or refactoring the block to have finer grained catches.

Catching and unwrapping RaiseException is a great idea, and should be included as part of the audit/refactor.

Contributor

tobias commented Oct 11, 2011

@baroquebobcat I wanted to revert to the blanket exception, since other exceptions in addition to RaiseException were slipping through, and affecting other JRuby consumers (Google App Engine, for example). I wanted to get 1.6.5 back to a known working state before auditing for all possible exceptions or refactoring the block to have finer grained catches.

Catching and unwrapping RaiseException is a great idea, and should be included as part of the audit/refactor.

@BanzaiMan

This comment has been minimized.

Show comment
Hide comment
@BanzaiMan

BanzaiMan Oct 20, 2011

Member

I merged Toby's patch.

Member

BanzaiMan commented Oct 20, 2011

I merged Toby's patch.

@BanzaiMan BanzaiMan closed this Oct 20, 2011

eregon added a commit that referenced this pull request Jun 15, 2015

Squashed 'spec/ruby/' changes from ee9abb8..1ac2212
1ac2212 Loosen the specs for Symbol#all_symbols increasing in size.
086a2b1 Fix bad Fixnum#- expectation.
636fc34 Merge pull request #77 from ruby/not-supported-on-opal
cdcb11a Fix order in SpecTCPServer#shutdown
89a5e8c Temporary fix for webrick < 2.2 which may not terminate the call to select
38692c5 Flags `x`, `n`, and `s` cannot be used in JavaScript regular expressions Regular expressions such as /(?ix:foo)/ are invalid in JavaScript
36ae046 Fix leaks and order dependent C-API specs
e9dc107 Fix Tempfile specs leaks
ffd01ce Fix socket and file leaks in Socket specs
1ae2b2d Fix leak in the Open3.popen3 spec and add an example
586f4db Start and stop the WEBrick server for each net/http spec
c5ae4d3 Make sure to close the socket after the FTP request
a7e875e Guard against IO.binread fd leak for older versions.
006fe2e Fix leaked threads in Thread specs
1f92316 Make the GC specs independent of whether the GC is enabled at entry.
9ad4483 Fix leaks in Kernel specs.
5822199 Do close @io to not leak the fd in IO#reopen.
3edfbae Fix a few fd leaks in IO specs.
8295a10 Fix many leaks, thanks to the new leak checker in MSpec.
4d96ab8 Create the CGI instance in the with the appropriate ENV variable
5857add Rewrite ARGF specs to not alter global state.
5034b02 Fix class variable spec to not be order-dependent
d17c24d Make sure to reset default_internal encoding in Dir.entries specs.
60225098 No need for after in ARGF.set_encoding since it shells out
b3e182e Fix various order-dependent specs.
60df767 Fix GC::Profiler.{enable,disable} specs.
b570eb3 Do not clear $LOADED_FEATURES and $LOAD_PATH
3486ace Fix Math.gamma test to depend only on local state.
c0bcb22 Fix flaky Module specs.
4a32dfa Make Dir spec not depend on the execution order.
d8e3429 Fix Kernel#{public_send,send} to not be order-dependent.
21c6ae1 Fix Module.constants spec leaking a constant.
ba05088 Fix remaining ARGF flaky tests by isolating ARGF.set_encoding.
54b3344 Fix flaky ARGF.lineno test.
78e9c7d Fix Thread#[]= leaking a thread-local on the main thread
f492595 Do not use global state with @method_args but just pass it along the method name.
2f3701a Add a comment to clarify the version guard.
c6cf315 [Truffle] Fix the guard for some keyword argument spec.
49576b4 Switch from using 'logname' to using 'id -un'.
bd3309d Merge pull request #75 from kachick/hash-fetch_values
66e5c08 Write Hash#fetch_values specs
e0cc72c Merge pull request #73 from ruby/fix-description
c8fc6d1 Fix description typo
4d0d42a Tempfile.new can take an encoding hash in 1.9.
ed001ec Move the unicode constant to another file
94dce9f Use the default arguments for mspec on Travis
a7b4d17 Allow 2.0.0 to fail until #72 is resolved.
253b92d Use MSpec without Bundler on travis
261855a Put a couple spec in quarantine a they conflict with bundler exec.
e5d0bd0 Run all specs by default
2a4ab31 Remove trailing spaces
e94542f Split the example in Integer#even? to remove guards.
be75461 Allow failures on OS X 2.0.0 until #72 is resolved.
109e4e8 Add notes about guards and style for contributing
8430044 Remove not_compliant_on :opal.
388a4ad Just ensure BasicObject can responds to #hash
4b2a19d Opal cannot support 'u', 'e', 's', and 'n' regexp flags
574d538 Add Array#bsearch examples using break in block
b93096a Construct Regexps with the constructor so that Opal can parse them.
e2ddc0c Opal cannot detect invalid UTF-8 byte sequences
fae5eb3 "cruel world".scan(:symbol) should not raise an error in Opal
6748b52 Opal cannot support regexps containing \A, \Z, and \z
0677951 Opal cannot support bignum
51b6594 Opal cannot parse the backslash line continuation
7050806 'hello'.match(:symbol) should not raise an error on Opal
2b66384 Specs that use numbers too large for JavaScript cannot be supported on Opal
f87cf7a Mark some unparsable by JS regexps as not_supported_on on Opal
124136f Regexp \A, \Z, \z, and \G cannot be supported on Opal
321d82a Guard invalid regexp flags for Opal
7e709a9 "'hello'.should_not eql(:hello)" cannot be supported on Opal
be16be6 "('stuff'.equal? 'stuff').should == false" cannot be supported on Opal
501e0b4 Missing case: "/[]9999".succ should be "/[]10000"
80ffe5d Avoid relying the type returned by #hash
6d31f3c Remove the last versioned file.
9aa3e41 Remove the Europe/Lisbon time zone change spec.

git-subtree-dir: spec/ruby
git-subtree-split: 1ac2212c39b665812fd2f4966fb5a68e019d51fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment