sirupsen / logrus

Structured, pluggable logging for Go.
MIT License
24.3k stars 2.26k forks source link

How to safely change Logger.Out on the fly? #1412

Closed JobberRT closed 5 months ago

JobberRT commented 6 months ago

Hi, I'm writing a rotation hook for logrus, but having an issue here: How to safely change the Logger.Out inside the Fire function?

Didn't go too deep into the source code of logrus, but here's my thoughts:

  1. Lock the output
  2. Close the old output
  3. Create the new output
  4. Set the new output to Logger.Out

Best-Effort code trying to do so:

// oldFile is an APPEND file, newFile is a TRUNCATE file
func TestRotate(t *testing.T) {
    logger := logrus.New()

    oldFile, err := os.OpenFile("./test.log", os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0755)
    if err != nil {
        panic(err)
    }

    logger.SetOutput(oldFile)

    time.Sleep(5 * time.Second)

    logger.SetOutput(func() io.Writer {
        _ = logger.Out.(*os.File).Close()

        newFile, err := os.OpenFile("./test.log", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0755)
        if err != nil {
            panic(err)
        }

        fmt.Println("new io create and old closed")
        return newFile
    }())

    _ = logger.Out.(*os.File).Close()

    time.Sleep(5 * time.Second)
}

The things I've discovered and tested

  1. If the Logger.Out is closed, nothing will be logged, and no panic will be throw
  2. Using this code, the execution priority is: immedialy executaed anonymous(Close the old Logger.Out and return a new Logger.Out) --> go inside logger.SetOutput --> logger.mu.lock --> sets the output for logger

Result and Problem

This code seems to work, it can successfully changed to a new write io(because the file is truncated and the oldFile is closed, and it still has messages writing into). But according to my two discovery, there's potential: When there are many stuff to log, and because logger.mu can't cover the anonymous function, some messages will be lost during the anonymous funciton close the old Logger.Out and logger.mu.lock then set the new logger.Out

github-actions[bot] commented 5 months ago

This issue is stale because it has been open for 30 days with no activity.

github-actions[bot] commented 5 months ago

This issue was closed because it has been inactive for 14 days since being marked as stale.