On 7/3/2013 4:38 AM, Sean Mullan wrote: > On 07/02/2013 02:49 PM, Vincent Ryan wrote: >> >> I see your point Sean. My motivation for wrapping the RuntimeException >> was to ensure >> that the failover mechanism (from OCSP to CRLs) did not get >> interrupted. But this is a >> case when the failover should be interrupted. I've filed a new bug to >> address the issue: >> I had a mis-understanding here. I thought the URL is coming from the X.509 certificate.
I agree with you that if it is a mis-configuration issue, it's better to throw RuntimeException. >> Bug: 8019627: RuntimeException gets obscured during OCSP cert >> revocation checking >> Webrev: http://cr.openjdk.java.net/~vinnie/8019627/webrev.00/ >> >> It simply re-throws RuntimeExceptions. > > I would prefer to just revert to the previous code (before 8019259). The > only checked exception thrown in that block is an IOException, so the > extra try/catch blocks for RuntimeException and Exception are unnecessary. > +1. Xuelei > --Sean > >> >> >> >> On 2 Jul 2013, at 16:19, Sean Mullan wrote: >> >>> On 07/01/2013 07:02 PM, Xuelei Fan wrote: >>>> On 7/1/2013 8:56 PM, Vincent Ryan wrote: >>>>> I think that wrapping a RuntimeException (in CPVE) is acceptable in >>>>> this case >>>>> because the goal is to activate the failover mechanism from OCSP to >>>>> CRL. >>>>> >>>>> Do you want RuntimeException to be re-thrown? >>>>> >>>> No. It is acceptable to me to wrap the RuntimeException. It is not a >>>> real runtime exception, but a wrong message. I prefer to use CPVE. >>> >>> Sorry I was on vacation yesterday and was unable to review this until >>> now. I'm not sure I like the fix. Catching a runtime exception and >>> rethrowing it as a checked exception is generally not a good idea. In >>> this case you have a situation where someone has misconfigured the >>> property to specify an OCSP responder. Hiding that exception and >>> allowing the revocation check to fallback to CRLs is dubious, even if >>> we still capture the exception in a log file. I think it should be >>> thrown as is, so the user knows about the issue immediately and can >>> fix the configuration. So, even though it is technically a change in >>> behavior from JDK 7, I think the current behavior is more correct, >>> and I doubt this would cause many compatibility issues since it is a >>> configuration error that needs manual intervention to be properly fixed. >>> >>> Thanks, >>> Sean >>> >>>> >>>> Xuelei >>>> >>>>> On 29 Jun 2013, at 01:53, Xuelei Fan wrote: >>>>> >>>>>> Looks fine to me. >>>>>> >>>>>> Hmm, it is a case to learn that RuntimeException should be token >>>>>> care of >>>>>> sometimes. >>>>>> >>>>>> Thanks, >>>>>> Xuelei >>>>>> >>>>>> On 6/29/2013 2:41 AM, Vincent Ryan wrote: >>>>>>> Hello, >>>>>>> >>>>>>> Please review the following JDK 8 fix: >>>>>>> >>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8019259 >>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8019259/webrev.00/ >>>>>>> >>>>>>> It corrects a problem during X.509 certificate revocation >>>>>>> checking where failover to using CRLs is not >>>>>>> performed in the case when a malformed URL has been supplied as >>>>>>> the URL of the OCSP responder. >>>>>>> The fix ensures all exceptions during OCSP are caught and wrapped >>>>>>> so that the failover mechanism >>>>>>> does not get skipped. >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>> >>>>> >>>> >>> >> >
