add error handling for loading of config.yml file

[adrienzag]

• Regarding the current code we have, it appears that when the config.yml file is initially loaded, it is done so within the reloadConfig() function. Upon examining the source code, it seems that this function does not explicitly throw any exceptions (see JavaPlugin.class) but somewhere inside it errors are handled.

• I attempted to replicate and tweak its behavior in WeatherByLocation.java to gain more control However, my attempts were unsuccessful. Despite utilizing a broad try-catch-block such as using (Exception e), I continued to encounter exceptions such as InvalidConfigurationException and others, apparently frontrunning my own error handling logic. It appeared that Bukkit's error handling took precedence over my implementation, testing was done with a misaligned test config file.

• During my search, I discovered that Bukkit employs a mechanism (although specifics remain unclear) where YamlConfiguration and its associated exceptions preemptively intercept mine during the loadConfiguration() process. To get around this I implemented a custom version of loadConfiguration(). This change gave us with greater control over handling the desired exceptions during the configuration loading process.

• So as of now it is unclear which exceptions should be caught ( current have FileNotFoundException,IOException, ParserException, InvalidConfigurationException ). Any feedback in this regard would be greatly appreciated.

• Additionally, it remains uncertain whether the server should terminate upon getting a bad configuration file - currently we are waiting 15 seconds then shutting down the server, otherwise we will run into the function that causes issues (reloadConfig() and or others) and we will see a bunch of exceptions).

• The way that the shutdown is done is not pretty so i am looking for alternatives, but this is the best i have for now.

• It is also probable that a custom class may be needed for the handling of config errors if we are to add more logic to the loadConfiguration() in order to avoid the WeatherByLocation.java being too busy with a slew of helper methods.

[scottbarnesg]

Hi @adrien2-public, thanks for submitting this!

On the overall approach, I think the desired behavior on config error should be to continue forward with some "sane defaults" and log a warning message, instead of shutting down the server. So, if there's an error reading the config file, the behavior should be:

• Log an aggressive warning message that there is an issue with configuration file, and that the plugin is falling back to default values.
• Get the server's location via ServerLocator (don't try to read from config).
• Set the update interval to the default of every 15 minutes.

[adrienzag]

Hi,

Sounds good, I have made some adjustments to reflect this.
In the ServerLocator.java class, I have added two functions:

• useDefaultLocator() and geolocateIPDefault(String ipAddress) that are used to get around having to interact with the config.yml file (which will generate an exception)

In the WeatherByLocation.java class, I have made some of the following changes:

• in the runStartupTasks() method i have extracted out the part where we reload the config file and set up the scheduler, we replaced it instead with an if-else function call that depends on whether or not the config file is good or not .
• currently under the executeBadConfig() method, we are still waiting for 15 seconds to allow user to see the message (not sure if this is desired), and we later make a call to Serverlocator.java class' useDefaultLocator() function - not too sure about the naming but this is what i have for now.

Let me know if there are any issues.

[adrienzag]

Hi Scott,
I just noticed that my most recent commit ( f849eef ) for error handling on config.yml files was not pushed through - I have taken a look at it again and am not sure where there may be issues.

Is it possible to let me know what needs to be fixed ?

Thanks