[rfc][icedtea-web] tuning of properties loading

[Andrew Azores]

On 01/24/2014 07:53 AM, Jiri Vanek wrote:
> On 01/23/2014 05:47 PM, Andrew Azores wrote:
>> On 01/23/2014 11:18 AM, Jiri Vanek wrote:
>>>
>>>>           } catch (IOException e) {
>>>> +            if (mandatory){
>>>> +                throw new 
>>>> ConfigurationException(e.getMessage());//unluckily
>>>> ConfigurationException can not have cause
>>>> +            }
>>>>
>>>>
>>>> ConfigurationException does have initCause(Throwable), does this 
>>>> not match what you need? It gets
>>>> this from its superclass NamingException which implements Throwable.
>>>
>>> Ah I overlooked!. Tahnx. Fixed.
>>
>> I still see this:
>>
>> +            if (mandatory){
>> +                throw new 
>> ConfigurationException(e.getMessage());//unluckily 
>> ConfigurationException
>> can not have cause
>> +            }
>>
>> why not use it here as well?
> ugh. ThanX!
>
>>
>> Also, this is just a style thing and feel free to ignore, but:
> :( I dont like it.. kept.
>>
>> +                ConfigurationException ce = new 
>> ConfigurationException("Invalid url to system
>> properties, which are mandatory");
>> +                ce.initCause(e);
>> +                throw ce;
>>
>> could also simply become:
>>
>> +                throw new ConfigurationException("Invalid url to 
>> system properties, which are
>> mandatory").initCause(e);
>>
>> since Throwable#initCause(Throwable) is defined to return "this" at 
>> the end.
>>
>>>>
>>>> +                //to be sure - we MUST die
>>>> -http://docs.oracle.com/javase/6/docs/technotes/guides/deployment/deployment-guide/properties.html 
>>>>
>>>> +            }catch(Throwable t){
>>>> +                //all exceptions are causing 
>>>> InstantiatizationError so this do it much more readble
>>>> + 
>>>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, t);
>>>> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
>>>> getMessage("RFailingToDefault"));
>>>> +                //try to survive this unlikely exception
>>>> +                config.resetToDefaults();
>>>>
>>>>
>>>> Hmm, what's the motivation behind adding this? What is there that's 
>>>> causing InstantiationErrors [1],
>>>> and if these occur, should it be trying to survive? Usually you 
>>>> don't want to be catching Errors
>>>> [2], so catch-all Throwable is even worse than catch-all Exception.
>>>
>>> a) the NoClassDefFoundError error is fatal and non recoverable
>>> b) no solid info *why* it happened  appeared (consumed any 
>>> NoClassDefFoundError?)
>>> c) we can run pretty well with defaults (and msut warn user - I'm 
>>> thinking also about poping up an
>>> dialogue in non headless mode, but I'm afraid that it cnabe to early 
>>> to (maybe another chnageset)
>>>
>>> I do not insist on throwable, If you wish I can lower to general 
>>> exception or whatever you wont.
>>> But defintley do not wont this static block to throw something
>
> Ok . I switched  to Exception. You sounds like don't likening 
> throwable :)
>>>
>>
>> Well, NoClassDefFoundError is a a subclass of Error, not Exception, 
>> so catching general Exception
>> will still not catch this! If you want this method to absolutely 
>> never throw anything without first
>> catching it on its own, then Throwable is the only choice you have.
>
> nn. You got it wrong.
>
> The NoClassDefFoundError is *caused* *by* any exception *from* the 
> staic initializer.   So I'm not cacthing NoClassDefFoundError, I'm 
> pereventing it to happen. I do not wont to survive something so fatal!
>
>>
>>>>
>>>> [1] 
>>>> http://docs.oracle.com/javase/7/docs/api/java/lang/InstantiationError.html
>>>
>>> Ugh sorry - it is java.lang.NoClassDefFoundError: Could not 
>>> initialize class
>>> net.sourceforge.jnlp.runtime.JNLPRuntime$DeploymentConfigurationHolder 
>>> what is thrown when static
>>> initializer throw some excption out.
>>
>> Ah okay this makes sense, I understand the motivation now. Catching 
>> all Throwable still sounds kind
>> of scary to me though, because this includes all kinds of Error 
>> subclasses that are really not meant
>> to be recoverable. Are you sure you want to be consuming and ignoring 
>> VirtualMachineError,
>> AssertionError, and LinkageError?
>
> as above. I think preventing the NoClassDefFoundError exception by 
> warning ( I added the pop up now)  by defaults is good thing to do.
>
> Thanx!
> J.

Okay, I think this last patch looks fine,

Thanks,

-- 
Andrew A