square / logcat

I CAN HAZ LOGZ?
Apache License 2.0
895 stars 21 forks source link

Better replace logger support #22

Open ericmaxwell2003 opened 1 year ago

ericmaxwell2003 commented 1 year ago

I don't think it's safe to call

1. LogcatLogger.uninstall()
1. LogcatLogger.install(AndroidLogcatLogger())

Because another thread could call logcat { "Well intended message" } between lines 1 and 2.

It's true that uninstall sets the logger to NoLog which says nothing is loggable. This would likely just drop any logcat{} logs that come in on the floor.. but... I was thinking that since the logcat{} calls delegate to the logger without locking on Logcat (which is an obvious choice and makes sense, you don't want to lock on that), but since we don't, I think it could be (looking at the implementation)

inline fun Any.logcat(
  priority: LogPriority = DEBUG,
  /**
   * If provided, the log will use this tag instead of the simple class name of `this` at the call
   * site.
   */
  tag: String? = null,
  message: () -> String
) {
  LogcatLogger.logger.let { logger ->

    //  IF THE LOG SWAP TO NoLog HAPPENS AFTER THIS if isLoggable LINE FIRES
    if (logger.isLoggable(priority)) {  

      val tagOrCaller = tag ?: outerClassSimpleNameInternalOnlyDoNotUseKThxBye()

      // BUT BEFORE THIS logger.log LINE FIRES
      logger.log(priority, tagOrCaller, message())
    }
  }
}

Then the NoLog will throw an exception

private object NoLog : LogcatLogger {
    override fun isLoggable(priority: LogPriority) = false

    override fun log(
      priority: LogPriority,
      tag: String,
      message: String
    ) = error("Should never receive any log")
  }
}

tl;dr;

I think we should offer a replaceLogger something like..

fun replaceLogger(newLogger: LogcatLogger) {
    synchronized(this) {
        uninstall()
        install(newLogger)
    }
}

And I think we should make NoLog gentler and something like

private object NoLog : LogcatLogger {
    override fun isLoggable(priority: LogPriority) = false
    override fun log(priority: LogPriority, tag: String, message: String) = Unit
  }
}
pyricau commented 1 year ago

If you need to have two logger implementations and swap these out, you can just make a logger implementation that does that within itself.

The API was never meant to be used as "uninstall(), install()" as a way to swap impls, and I'd rather not relax NoLog just for a use case that the APIs aren't meant to support and that AFAIK can be implemented with a custom logger