libs: Handle SIGHUP explicitly inside autofile (#2480)

* handle SIGHUP explicitly inside autofile
Refs #2260
* libs: Use consistent channel suffix

libs/autofile/autofile.go

@@ -2,7 +2,9 @@ package autofile
 
 import (
 	"os"
+	"os/signal"
 	"sync"
+	"syscall"
 	"time"
 
 	cmn "github.com/tendermint/tendermint/libs/common"
@@ -32,50 +34,70 @@ if err != nil {
 */
 
 const (
-	autoFileOpenDuration = 1000 * time.Millisecond
-	autoFilePerms        = os.FileMode(0600)
+	autoFileClosePeriod = 1000 * time.Millisecond
+	autoFilePerms       = os.FileMode(0600)
 )
 
-// Automatically closes and re-opens file for writing.
+// AutoFile automatically closes and re-opens file for writing. The file is
+// automatically setup to close itself every 1s and upon receiving SIGHUP.
+//
 // This is useful for using a log file with the logrotate tool.
 type AutoFile struct {
-	ID            string
-	Path          string
-	ticker        *time.Ticker
-	tickerStopped chan struct{} // closed when ticker is stopped
-	mtx           sync.Mutex
-	file          *os.File
+	ID   string
+	Path string
+
+	closeTicker      *time.Ticker
+	closeTickerStopc chan struct{} // closed when closeTicker is stopped
+	hupc             chan os.Signal
+
+	mtx  sync.Mutex
+	file *os.File
 }
 
-func OpenAutoFile(path string) (af *AutoFile, err error) {
-	af = &AutoFile{
-		ID:            cmn.RandStr(12) + ":" + path,
-		Path:          path,
-		ticker:        time.NewTicker(autoFileOpenDuration),
-		tickerStopped: make(chan struct{}),
+// OpenAutoFile creates an AutoFile in the path (with random ID). If there is
+// an error, it will be of type *PathError or *ErrPermissionsChanged (if file's
+// permissions got changed (should be 0600)).
+func OpenAutoFile(path string) (*AutoFile, error) {
+	af := &AutoFile{
+		ID:               cmn.RandStr(12) + ":" + path,
+		Path:             path,
+		closeTicker:      time.NewTicker(autoFileClosePeriod),
+		closeTickerStopc: make(chan struct{}),
 	}
-	if err = af.openFile(); err != nil {
-		return
+	if err := af.openFile(); err != nil {
+		af.Close()
+		return nil, err
 	}
-	go af.processTicks()
-	sighupWatchers.addAutoFile(af)
-	return
+
+	// Close file on SIGHUP.
+	af.hupc = make(chan os.Signal, 1)
+	signal.Notify(af.hupc, syscall.SIGHUP)
+	go func() {
+		for range af.hupc {
+			af.closeFile()
+		}
+	}()
+
+	go af.closeFileRoutine()
+
+	return af, nil
 }
 
 func (af *AutoFile) Close() error {
-	af.ticker.Stop()
-	close(af.tickerStopped)
-	err := af.closeFile()
-	sighupWatchers.removeAutoFile(af)
-	return err
+	af.closeTicker.Stop()
+	close(af.closeTickerStopc)
+	if af.hupc != nil {
+		close(af.hupc)
+	}
+	return af.closeFile()
 }
 
-func (af *AutoFile) processTicks() {
+func (af *AutoFile) closeFileRoutine() {
 	for {
 		select {
-		case <-af.ticker.C:
+		case <-af.closeTicker.C:
 			af.closeFile()
-		case <-af.tickerStopped:
+		case <-af.closeTickerStopc:
 			return
 		}
 	}
@@ -89,6 +111,7 @@ func (af *AutoFile) closeFile() (err error) {
 	if file == nil {
 		return nil
 	}
+
 	af.file = nil
 	return file.Close()
 }

libs/autofile/autofile_test.go

@@ -3,7 +3,6 @@ package autofile
 import (
 	"io/ioutil"
 	"os"
-	"sync/atomic"
 	"syscall"
 	"testing"
 	"time"
@@ -37,13 +36,10 @@ func TestSIGHUP(t *testing.T) {
 	require.NoError(t, err)
 
 	// Send SIGHUP to self.
-	oldSighupCounter := atomic.LoadInt32(&sighupCounter)
 	syscall.Kill(syscall.Getpid(), syscall.SIGHUP)
 
 	// Wait a bit... signals are not handled synchronously.
-	for atomic.LoadInt32(&sighupCounter) == oldSighupCounter {
-		time.Sleep(time.Millisecond * 10)
-	}
+	time.Sleep(time.Millisecond * 10)
 
 	// Write more to the file.
 	_, err = af.Write([]byte("Line 3\n"))
@@ -87,7 +83,4 @@ func TestOpenAutoFilePerms(t *testing.T) {
 	} else {
 		t.Errorf("unexpected error %v", e)
 	}
-
-	err = af.Close()
-	require.NoError(t, err)
 }

libs/autofile/sighup_watcher.go

@@ -1,69 +0,0 @@
-package autofile
-
-import (
-	"os"
-	"os/signal"
-	"sync"
-	"sync/atomic"
-	"syscall"
-)
-
-func init() {
-	initSighupWatcher()
-}
-
-var sighupWatchers *SighupWatcher
-var sighupCounter int32 // For testing
-
-func initSighupWatcher() {
-	sighupWatchers = newSighupWatcher()
-
-	hup := make(chan os.Signal, 1)
-	signal.Notify(hup, syscall.SIGHUP)
-
-	quit := make(chan os.Signal, 1)
-	signal.Notify(quit, os.Interrupt, syscall.SIGTERM)
-
-	go func() {
-		select {
-		case <-hup:
-			sighupWatchers.closeAll()
-			atomic.AddInt32(&sighupCounter, 1)
-		case <-quit:
-			return
-		}
-	}()
-}
-
-// Watchces for SIGHUP events and notifies registered AutoFiles
-type SighupWatcher struct {
-	mtx       sync.Mutex
-	autoFiles map[string]*AutoFile
-}
-
-func newSighupWatcher() *SighupWatcher {
-	return &SighupWatcher{
-		autoFiles: make(map[string]*AutoFile, 10),
-	}
-}
-
-func (w *SighupWatcher) addAutoFile(af *AutoFile) {
-	w.mtx.Lock()
-	w.autoFiles[af.ID] = af
-	w.mtx.Unlock()
-}
-
-// If AutoFile isn't registered or was already removed, does nothing.
-func (w *SighupWatcher) removeAutoFile(af *AutoFile) {
-	w.mtx.Lock()
-	delete(w.autoFiles, af.ID)
-	w.mtx.Unlock()
-}
-
-func (w *SighupWatcher) closeAll() {
-	w.mtx.Lock()
-	for _, af := range w.autoFiles {
-		af.closeFile()
-	}
-	w.mtx.Unlock()
-}