RFR: 8075810: LogManager.readConfiguration may throw undocumented IllegalArgumentException

Fri Mar 27 16:20:16 UTC 2015

Hi Daniel,

Looks good..  Let me know when you have your draft and I will make myself a reviewer of your ccc

Best
Lance
On Mar 27, 2015, at 12:18 PM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:

> Hi,
> 
> I received private feedback that using plain IOException
> might be preferable to using CharConversionException, as
> that doesn't exactly reflect what is going on here.
> 
> So here is my new webrev - which includes the minor formatting
> issues found by Jason, but reverts to using plain IOException:
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.02
> 
> If I don't hear negative feedback that's what I'm going to
> present to CCC.
> 
> best regards,
> 
> -- daniel
> 
> On 25/03/15 12:41, Daniel Fuchs wrote:
>> Thanks for the review Jason!
>> 
>> On 24/03/15 18:01, Jason Mehrens wrote:
>>> Hi Daniel,
>>> 
>>> Looks good.  The only other alternative would be to use
>>> java.io.CharConversionException over IOException.  We could even
>>> consider dropping the cause because the subclass of I/O exception
>>> would convey the same meaning.
>> 
>> Here is a an updated webrev where I use
>> java.io.CharConversionException instead of plain IOException.
>> 
>> http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.01/
>> 
>> Note: I had a look in the JDK sources and CharConversionException
>> doesn't appear to be widely used. Is this the better alternative?
>> I'd be happy with both (webrev.01 or webrev.00).
>> Using CharConversionException means that we have to trust that
>> Properties.load will obey its spec and only throw IAE in case
>> of character conversion issues.
>> 
>>> Minor formatting issues with a missing space after the catch keyword
>> 
>> Done. Thanks for catching it.
>> 
>>> and missing a tab ahead of 'props.load'
>> 
>> That is an artifact of how webrev calls diff I think.
>> If you look at the frames version you will see that the tab is here.
>> 
>> best regards,
>> 
>> -- daniel
>> 
>>> 
>>> Jason
>>> 
>>> 
>>> ----------------------------------------
>>>> Date: Tue, 24 Mar 2015 16:06:47 +0100
>>>> From: daniel.fuchs at oracle.com
>>>> To: lance.andersen at oracle.com
>>>> Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw
>>>> undocumented IllegalArgumentException
>>>> CC: core-libs-dev at openjdk.java.net
>>>> 
>>>> Hi Lance,
>>>> 
>>>> Thanks for the review!
>>>> 
>>>> On 24/03/15 16:01, Lance Andersen wrote:
>>>>> Hi Daniel,
>>>>> 
>>>>> This looks OK but I might suggest clarifying that the cause could be
>>>>> set
>>>>> in the javadoc. Now that being said, I am not sure how consistent we
>>>>> are across the JDK or just make the assumption people will check the
>>>>> cause if they desire.
>>>> 
>>>> Hmmm. I have the feeling that the best place for that would be i the
>>>> release notes - rather than in the API doc (which reminds me I should
>>>> plan to add some release note text to the issue).
>>>> 
>>>>> As far as backporting, unless it really needed, I would probably would
>>>>> not as a change such as this typically should be done as part of an
>>>>> errata/MR (due to change of behavior and it is not that big of an
>>>>> issue).
>>>> 
>>>> Right, my thinking too. Thanks for sharing your opinion!
>>>> 
>>>> best regards,
>>>> 
>>>> -- daniel
>>>> 
>>>>> 
>>>>> Best
>>>>> Lance
>>>>> On Mar 24, 2015, at 10:42 AM, Daniel Fuchs <daniel.fuchs at oracle.com
>>>>> <mailto:daniel.fuchs at oracle.com>> wrote:
>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> Please find below a fix for
>>>>>> 
>>>>>> 8075810: LogManager.readConfiguration may throw
>>>>>> undocumented IllegalArgumentException
>>>>>> 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8075810
>>>>>> 
>>>>>> The proposed fix is to catch the IllegalArgumentException and
>>>>>> wrap it in an IOException, since LogManager.readConfiguration
>>>>>> is specified to throw IOException "if there are problems reading
>>>>>> from the stream."
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.00/
>>>>>> 
>>>>>> Initial discussion around the issue can be seen here:
>>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032348.html
>>>>>> 
>>>>>> 
>>>>>> Question for reviewers: I will log a CCC for this - but I am wondering
>>>>>> whether I should plan to backport the fix to earlier release?
>>>>>> 
>>>>>> 
>>>>>> best regards,
>>>>>> 
>>>>>> -- daniel
>>>>> 
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>>> 
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
>>>>> Andersen|
>>>>> Principal Member of Technical Staff | +1.781.442.2037
>>>>> Oracle Java Engineering
>>>>> 1 Network Drive
>>>>> Burlington, MA 01803
>>>>> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>>>>> 
>>>>> 
>>>>> 
>>>> 
>> 
> 

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com