From f05c9ffde8b9407c01f57d9793684752609c7cae Mon Sep 17 00:00:00 2001
From: Joe Henke <jhenke@palantir.com>
Date: Fri, 15 Mar 2019 14:27:18 -0700
Subject: [PATCH] Avoid persistent mill goroutine

Previously, each lumberjack logger starts its own mill goroutine the first time
that the logger writes to a file, and the goroutine is never stopped. The
project has an open pull request,
https://github.com/natefinch/lumberjack/pull/80, to stop the goroutine when the
logger is closed. However, even with that design, each open logger will always
have a mill goroutine that is active (even if it is not performing work). It
also means that a logger will never be garbage-collected (and hence its file
will never be closed) unless it is explicitly closed. Furthemore, even if a
logger is explicitly closed, if Write is called on any references to that
logger, that will restart the persistent mill goroutine and reintroduce the
leaked file handle.

This change modifies the mill goroutine logic. It still maintains the overall
design of synchronously rotating and asynchronously deleting and compressing
(milling) the old, rotated log files, and that if a request to mill comes in
while one call is already happening, another mill will be executed after the
current one to ensure any newly rotated files are not missed. There is at most
one active mill goroutine which is in the process of performing a single pass
of deletions and compressions of rotated log files, and at most one queued mill
goroutine which is just waiting on a mutex. Now, each mill goroutine exits once
a single pass is done and future calls to mill will start a new goroutine (if
one is not already queued.)

This modification allows for lumberjack loggers to be garbage-collected, which
allows it to close file handles gracefully without an explicit call to "close".

The downside to this new approach is in the case where rotations are constantly
happening, rather than having a single goroutine simply process each mill pass,
a new goroutine must be started for each pass.

---

We have an internal fork of lumberjack that contains this change that we are
using in production.

An alternative approach was sketched after the fact that is not in use
internally but would have at most one mill goroutine per logger at any time,
rather than one active and one queued as in this approach. This alternative
design is shown here:

https://github.com/jdhenke/lumberjack/pull/2
---
 lumberjack.go | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/lumberjack.go b/lumberjack.go
index a47b7f0..1485efc 100644
--- a/lumberjack.go
+++ b/lumberjack.go
@@ -32,6 +32,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 )
 
@@ -111,8 +112,8 @@ type Logger struct {
 	file *os.File
 	mu   sync.Mutex
 
-	millCh    chan bool
-	startMill sync.Once
+	needsMill int32
+	millMu    sync.Mutex
 }
 
 var (
@@ -302,6 +303,20 @@ func (l *Logger) filename() string {
 // files are removed, keeping at most l.MaxBackups files, as long as
 // none of them are older than MaxAge.
 func (l *Logger) millRunOnce() error {
+
+	// ensure only one mill worker is working at once
+	l.millMu.Lock()
+	defer l.millMu.Unlock()
+
+	// decrement needsMill to 0 to allow any calls to mill() that
+	// occur while this call to millRunOnce() is running to queue
+	// another call to millRunOnce() that will handle any files that
+	// this call may have missed in a race. at most one additional
+	// goroutine will ever be queued because once the first one is
+	// queued, needsMill will be 1 until that queued goroutine is
+	// run and hits this line itself.
+	atomic.StoreInt32(&l.needsMill, 0)
+
 	if l.MaxBackups == 0 && l.MaxAge == 0 && !l.Compress {
 		return nil
 	}
@@ -373,25 +388,12 @@ func (l *Logger) millRunOnce() error {
 	return err
 }
 
-// millRun runs in a goroutine to manage post-rotation compression and removal
-// of old log files.
-func (l *Logger) millRun() {
-	for _ = range l.millCh {
-		// what am I going to do, log this?
-		_ = l.millRunOnce()
-	}
-}
-
 // mill performs post-rotation compression and removal of stale log files,
-// starting the mill goroutine if necessary.
+// starting a mill goroutine if necessary.
 func (l *Logger) mill() {
-	l.startMill.Do(func() {
-		l.millCh = make(chan bool, 1)
-		go l.millRun()
-	})
-	select {
-	case l.millCh <- true:
-	default:
+	// queue a call to millRunOnce() if one has not already been queued
+	if atomic.CompareAndSwapInt32(&l.needsMill, 0, 1) {
+		go l.millRunOnce()
 	}
 }