I'm working with a new go service and I have a SetupLogger
utility function that creates a new instance of go-kit's logging struct log.Logger
.
Is this method safe to invoke from code that's handling requests inside separate go-routines?
package utils
import (
"fmt"
"github.com/go-kit/kit/log"
"io"
"os"
"path/filepath"
)
// If the environment-specified directory for writing log files exists, open the existing log file
// if it already exists or create a log file if no log file exists.
// If the environment-specified directory for writing log files does not exist, configure the logger
// to log to process stdout.
// Returns an instance of go-kit logger
func SetupLogger() log.Logger {
var logWriter io.Writer
var err error
LOG_FILE_DIR := os.Getenv("CRAFT_API_LOG_FILE_DIR")
LOG_FILE_NAME := os.Getenv("CRAFT_API_LOG_FILE_NAME")
fullLogFilePath := filepath.Join(
LOG_FILE_DIR,
LOG_FILE_NAME,
)
if dirExists, _ := Exists(&ExistsOsCheckerStruct{}, LOG_FILE_DIR); dirExists {
if logFileExists, _ := Exists(&ExistsOsCheckerStruct{}, fullLogFilePath); !logFileExists {
os.Create(fullLogFilePath)
}
logWriter, err = os.OpenFile(fullLogFilePath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
if err != nil {
fmt.Println("Could not open log file. ", err)
}
} else {
logWriter = os.Stdout
}
return log.NewContext(log.NewJSONLogger(logWriter)).With(
"timestamp", log.DefaultTimestampUTC,
"caller", log.DefaultCaller,
)
}
Since your setting up of your Logger only involves library instantiation, creating a log file if it doesn't exist, opening the log file and no writing involved there will be no problem calling it from different go-routines since the shared data is not getting tampered with.
Side note: Design wise it makes sense (assuming Logger is writing to the same file) to pass around the only instantiated instance of Logger for logging which would prevent two go-routines calling your setup function at the same time.
First recommendation: Use the -race
flag to go build
and go test
. It will almost always be able to tell you if you have a race condition. Although it might not in this case since you could end up calling your os.Create()
and your os.OpenFile()
simultaneously.
So, second recommendation is to avoid, if at all possible, the "If it exists/matches/has permissions Then open/delete/whatever" pattern.
That pattern leads to the TOCTTOU (Time of Check To Time Of Use) bug, which is often a security bug and can at the very least lead to data loss.
To avoid it either wrap the check and use into the same mutex, or use atomic operations, such as an OpenFile call that creates the file or returns an error if it already existed (although to be technical, its locked inside the OS kernel. Just like how atomic CPU ops are locked in the hardware bus.).
In your case here I am not quite sure why you have two Open calls since it looks like just one would do the job.