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

[Jiri Vanek]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tuningOfPropertiesLoading3.patch
Type: text/x-patch
Size: 8495 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140124/8749bbf0/tuningOfPropertiesLoading3.patch