Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better replace logger support #22

Open
ericmaxwell2003 opened this issue Oct 19, 2023 · 1 comment
Open

Better replace logger support #22

ericmaxwell2003 opened this issue Oct 19, 2023 · 1 comment

Comments

@ericmaxwell2003
Copy link

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
Copy link
Member

pyricau commented Nov 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants