RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Claes Redestad]

Hi again,

please review the latest revision, which fixes a small discrepancy where Security.getProperty was used instead of System.getProperty (implicitly called by Integer.getInteger in the original code):

http://cr.openjdk.java.net/~mduigou/JDK-8040837/1/webrev/

/Claes

On 2014-04-18 22:52, Mike Duigou wrote:
> Updated webrev from Claes:
>
> http://cr.openjdk.java.net/~mduigou/JDK-8040837/1/webrev/
>
> On Apr 18 2014, at 11:02 , claes.redestad <claes.redestad at oracle.com> wrote:
>
>> Looks like you're right, unfortunately. Seems I confused System.getProperty and Security.getProperty, and of course they differ subtly. I'll fix it as soon as I can, but I'm out for a few hours..
>>
>> /Claes
>>
>>
>> -------- Originalmeddelande --------
>> Från: Bernd Eckenfels
>> Datum:18-04-2014 19:06 (GMT+01:00)
>> Till: Michael McMahon
>> Kopia: Mike Duigou , claes.redestad at oracle.com
>> Rubrik: Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy
>>
>> Hello,
>>
>> I think the updated revision also uses Security.getProperty for the
>> fallbacks, I think this is wrong, they are read from the System
>> properties. (can be read with Integer.getProperty which uses internally
>> decode).
>>
>> (and sorry for the big discussion on such a small patch :)
>>
>> Gruss
>> Bernd
>>
>> Am Fri, 18 Apr 2014 17:57:04 +0100 schrieb Michael McMahon
>> <michael.x.mcmahon at oracle.com>:
>>
>>> Thanks Mike (and adding Claes back in)
>>>
>>> Is there a reason to use both Integer.decode() and Integer.valueOf()?
>>>
>>> Michael
>>>
>>> On 18/04/14 17:44, Mike Duigou wrote:
>>>> Claes tidied things up to produce a workable patch:
>>>>
>>>>> Here is the updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8040837/0/webrev/
>>>>>
>>>>> I will push it to jdk9/dev/jdk on Friday before COB for Claes
>>>>> unless I hear objections.
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Mike
>>>> On Apr 18 2014, at 09:27 , Michael McMahon
>>>> <michael.x.mcmahon at oracle.com> wrote:
>>>>
>>>>> I think it would be an improvement to combine these doPrivileged()
>>>>> blocks as suggested, though your patch needs work Bernd. For
>>>>> instance, the multi-catch doesn't work. Also the
>>>>> PrivilegedAction<> type is wrong.
>>>>>
>>>>> If someone wants to update it, then we can use that. Otherwise,
>>>>> we'll go with the original suggested change.
>>>>>
>>>>> Thanks
>>>>> Michael
>>>>>
>>>>> On 17/04/14 21:28, Bernd Eckenfels wrote:
>>>>>> Am Thu, 17 Apr 2014 21:50:23 +0200
>>>>>> schrieb Bernd Eckenfels <bernd-2014 at eckenfels.net>:
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> I would propose to use Integer.valueOf(tmp) instead, but looking
>>>>>>> at the context I think it is even better to skip this and the
>>>>>>> following null check with Integer.parseInt().
>>>>>> This is even shorter and it reduces the privileged actions
>>>>>> invocations:
>>>>>>
>>>>>>          Integer tmp = java.security.AccessController.doPrivileged
>>>>>> ( new PrivilegedAction<String>() {
>>>>>>              public String run() {
>>>>>>                  try {
>>>>>>                      String tmpString =
>>>>>> Security.getProperty(cachePolicyProp); if (tmpString != null)
>>>>>> return Integer.valueOf(tmpString); } catch (NumberFormatException
>>>>>> | IllegalArgumentException ignored) { }
>>>>>>
>>>>>>                  try {
>>>>>>                      return
>>>>>> Integer.getInteger(cachePolicyPropFallback); } catch
>>>>>> (NumberFormatException | IllegalArgumentException ignored) { }
>>>>>>
>>>>>>                  return null;
>>>>>>              }
>>>>>>          });
>>>>>>
>>>>>>           if (tmp != null) {
>>>>>>               cachePolicy = tmp.intValue();
>>>>>>               if (cachePolicy < 0) {
>>>>>>                   cachePolicy = FOREVER;
>>>>>>               }
>>>>>>               propertySet = true;
>>>>>>           } else {
>>>>>>               /* No properties defined for positive caching. If
>>>>>> there is no
>>>>>>                * security manager then use the default positive
>>>>>> cache value. */ if (System.getSecurityManager() == null) {
>>>>>>                   cachePolicy = DEFAULT_POSITIVE;
>>>>>>               }
>>>>>>           }
>>>>>>
>>>>>> NB: this will