Two questions: Swallowing exceptions? Accepting nullable Throwables?

[darioseidl]

Hello,

Thank you for this library! I love how it provides true lazy-evaluation. Prettier and, afaict, even faster than the idiomatic way of Slf4j log.info("foo: {}", bar).

However, I noticed the toStringSafe method in MessageInvoker.kt:

internal inline fun (() -> Any?).toStringSafe(): String {
    try {
        return invoke().toString()
    } catch (e: Exception) {
        return "Log message invocation failed: $e"
    }
}

This means that exceptions from a logging lambda will be caught and swallowed. Just the exception message will be logged, on the same log level as the base statement. E.g.

log.info{"Test ${"x".toInt()}"}   

prints, on level INFO

Log message invocation failed: java.lang.NumberFormatException: For input string: "x"

I'm wondering if this is desirable? It should be rare to actually get any exception from a log statement, but when it happens, I would rather see the exception fall through so that it can be handled by whatever root exception handling is in place. In our current project, we would log on level ERROR and send a mail, to make sure that no exception goes unnoticed.

What do you think? Is there a way to skip or replace the toStringSafe?

--

Another thing, not very important at all, but is there a reason that the methods of Klogger.kt don't accept nullable Throwables?

I would like to do, for example, the following, in an exception handler implementation

val ex: Throwable? = httpServletRequest.getAttribute(DispatcherServlet.EXCEPTION_ATTRIBUTE) as? Throwable
log.error (ex) { "Error $status dispatched for request $requestURI" } 

That's not possible though, since ex is nullable, and KLogger.error expects an Throwable. The underlying Slf4j logger would accept null and simply handle it as if no exception was passed to it. so I don't see a reason why Klogger could not accept a Throwable? as well.

[oshai]

Regarding the first issue see the discussion here: #22

Regarding the second issue, a pull request is welcome!

[darioseidl]

Thanks for the response! I understand this is a question of preference (or context), some people prefer to be fail-safe, while I'd rather fail-fast. The behavior is also different from Slf4j, compare:

log.underlyingLogger.info("Test: {}", "x".toInt())

with

log.info{"Test ${"x".toInt()}"}

The former throws the exception, the latter doesn't. So I think this should at least be mentioned somewhere in the readme or documentation, otherwise it could lead to some real head scratchers.

I created a pull request for the nullable throwables.

[oshai]

usually, I also prefer fail-fast than fail-safe. However, I think in this case it is better to be consistent with other logging frameworks. the exception you described above in "x".toInt() is not part of the framework but happened before the framework is called.

[oshai]

version 1.5.4 was released with nullable throwables (#39)

[darioseidl]

Ah, yes, you're right, the logging framework doesn't even even see the exception in the first example.

Thank you for the quick release!