search

jdiazcano / cfg4k

Flexible and easy to use config library written in kotlin
Apache License 2.0
80 stars 6 forks source link

FileChangeReloadStrategy thread dies on Exception: no error handler #43

Closed rocketraman closed 6 years ago

rocketraman commented 6 years ago

The FileChangeReloadStrategy has no exception handler. This means that if an exception is thrown when the underlying config is updated -- for example, if the config is updated with invalid syntax -- the reload thread dies and no more config reloads are handled until the ConfigProvider is constructed again.

Instead, I think I would log a warning in this situation, but continue to monitor the file for more changes. However, I also note that there is no logging in the lib now, so an alternate approach may be to trigger another listener on provider in this case e.g. addReloadErrorListener?

jdiazcano commented 6 years ago

I will for sure add the reloadErrorListener feature (almost for sure today). I actually didn't think about configuration mistakes etc.

This will be the signature of the function inside the ConfigProvider interface.

    /**
     * Adds a reload listener that will be called when there is an exception in the reload process.
     */
    fun addReloadErrorListener(listener: () -> Unit)

For the logging... I might do it but that is not one of my priorities right now. With the experience I have seen that the logging for a (I'd say) relatively small library like this one adds nothing but overhead for the real application log so I decided to go with the listeners approach where each one can react how they want to the different events like reload, error reloading etc.

jdiazcano commented 6 years ago

So I have added the listener, it is currently in master.

    /**
     * Adds a reload listener that will be called when there is an exception in the reload process.
     */
    fun addReloadErrorListener(listener: (Exception) -> Unit)

This is the final signature and for the logging you can open a new issue to be discussed there, please!

rocketraman commented 6 years ago

This is the final signature and for the logging you can open a new issue to be discussed there, please!

No need I think... I agree with your idea that logging is unnecessary and listeners are the way to go. Thanks!