From efa6940001c8da02a58552bd55ac84cbafbbe6f3 Mon Sep 17 00:00:00 2001 From: Kevin Wan Date: Fri, 9 Jun 2023 22:50:59 +0800 Subject: [PATCH] chore: improve logx gzip (#3332) --- core/logx/fs.go | 40 +++++++++++ core/logx/rotatelogger.go | 34 ++++++---- core/logx/rotatelogger_test.go | 117 +++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 12 deletions(-) create mode 100644 core/logx/fs.go diff --git a/core/logx/fs.go b/core/logx/fs.go new file mode 100644 index 00000000..5834cd92 --- /dev/null +++ b/core/logx/fs.go @@ -0,0 +1,40 @@ +package logx + +import ( + "io" + "os" +) + +var fileSys realFileSystem + +type ( + fileSystem interface { + Close(closer io.Closer) error + Copy(writer io.Writer, reader io.Reader) (int64, error) + Create(name string) (*os.File, error) + Open(name string) (*os.File, error) + Remove(name string) error + } + + realFileSystem struct{} +) + +func (fs realFileSystem) Close(closer io.Closer) error { + return closer.Close() +} + +func (fs realFileSystem) Copy(writer io.Writer, reader io.Reader) (int64, error) { + return io.Copy(writer, reader) +} + +func (fs realFileSystem) Create(name string) (*os.File, error) { + return os.Create(name) +} + +func (fs realFileSystem) Open(name string) (*os.File, error) { + return os.Open(name) +} + +func (fs realFileSystem) Remove(name string) error { + return os.Remove(name) +} diff --git a/core/logx/rotatelogger.go b/core/logx/rotatelogger.go index 1b480eb4..c8ef43a7 100644 --- a/core/logx/rotatelogger.go +++ b/core/logx/rotatelogger.go @@ -4,7 +4,6 @@ import ( "compress/gzip" "errors" "fmt" - "io" "log" "os" "path" @@ -406,7 +405,7 @@ func (l *RotateLogger) write(v []byte) { func compressLogFile(file string) { start := time.Now() Infof("compressing log file: %s", file) - if err := gzipFile(file); err != nil { + if err := gzipFile(file, fileSys); err != nil { Errorf("compress error: %s", err) } else { Infof("compressed log file: %s, took %s", file, time.Since(start)) @@ -421,26 +420,37 @@ func getNowDateInRFC3339Format() string { return time.Now().Format(fileTimeFormat) } -func gzipFile(file string) error { - in, err := os.Open(file) +func gzipFile(file string, fsys fileSystem) (err error) { + in, err := fsys.Open(file) if err != nil { return err } + defer func() { + if e := fsys.Close(in); e != nil { + Errorf("failed to close file: %s, error: %v", file, e) + } + if err == nil { + // only remove the original file when compression is successful + err = fsys.Remove(file) + } + }() - out, err := os.Create(fmt.Sprintf("%s%s", file, gzipExt)) + out, err := fsys.Create(fmt.Sprintf("%s%s", file, gzipExt)) if err != nil { return err } - defer out.Close() + defer func() { + e := fsys.Close(out) + if err == nil { + err = e + } + }() w := gzip.NewWriter(out) - if _, err = io.Copy(w, in); err != nil { - return err - } else if err = w.Close(); err != nil { + if _, err = fsys.Copy(w, in); err != nil { + // failed to copy, no need to close w return err } - in.Close() - - return os.Remove(file) + return fsys.Close(w) } diff --git a/core/logx/rotatelogger_test.go b/core/logx/rotatelogger_test.go index 6079c3b1..68269f6e 100644 --- a/core/logx/rotatelogger_test.go +++ b/core/logx/rotatelogger_test.go @@ -1,9 +1,12 @@ package logx import ( + "errors" + "io" "os" "path" "path/filepath" + "sync/atomic" "syscall" "testing" "time" @@ -429,6 +432,70 @@ func TestRotateLoggerWithSizeLimitRotateRuleWrite(t *testing.T) { logger.write([]byte(`baz`)) } +func TestGzipFile(t *testing.T) { + err := errors.New("any error") + + t.Run("gzip file open failed", func(t *testing.T) { + fsys := &fakeFileSystem{ + openFn: func(name string) (*os.File, error) { + return nil, err + }, + } + assert.ErrorIs(t, err, gzipFile("any", fsys)) + assert.False(t, fsys.Removed()) + }) + + t.Run("gzip file create failed", func(t *testing.T) { + fsys := &fakeFileSystem{ + createFn: func(name string) (*os.File, error) { + return nil, err + }, + } + assert.ErrorIs(t, err, gzipFile("any", fsys)) + assert.False(t, fsys.Removed()) + }) + + t.Run("gzip file copy failed", func(t *testing.T) { + fsys := &fakeFileSystem{ + copyFn: func(writer io.Writer, reader io.Reader) (int64, error) { + return 0, err + }, + } + assert.ErrorIs(t, err, gzipFile("any", fsys)) + assert.False(t, fsys.Removed()) + }) + + t.Run("gzip file last close failed", func(t *testing.T) { + var called int32 + fsys := &fakeFileSystem{ + closeFn: func(closer io.Closer) error { + if atomic.AddInt32(&called, 1) > 2 { + return err + } + return nil + }, + } + assert.NoError(t, gzipFile("any", fsys)) + assert.True(t, fsys.Removed()) + }) + + t.Run("gzip file remove failed", func(t *testing.T) { + fsys := &fakeFileSystem{ + removeFn: func(name string) error { + return err + }, + } + assert.Error(t, err, gzipFile("any", fsys)) + assert.True(t, fsys.Removed()) + }) + + t.Run("gzip file everything ok", func(t *testing.T) { + fsys := &fakeFileSystem{} + assert.NoError(t, gzipFile("any", fsys)) + assert.True(t, fsys.Removed()) + }) +} + func BenchmarkRotateLogger(b *testing.B) { filename := "./test.log" filename2 := "./test2.log" @@ -480,3 +547,53 @@ func BenchmarkRotateLogger(b *testing.B) { } }) } + +type fakeFileSystem struct { + removed int32 + closeFn func(closer io.Closer) error + copyFn func(writer io.Writer, reader io.Reader) (int64, error) + createFn func(name string) (*os.File, error) + openFn func(name string) (*os.File, error) + removeFn func(name string) error +} + +func (f *fakeFileSystem) Close(closer io.Closer) error { + if f.closeFn != nil { + return f.closeFn(closer) + } + return nil +} + +func (f *fakeFileSystem) Copy(writer io.Writer, reader io.Reader) (int64, error) { + if f.copyFn != nil { + return f.copyFn(writer, reader) + } + return 0, nil +} + +func (f *fakeFileSystem) Create(name string) (*os.File, error) { + if f.createFn != nil { + return f.createFn(name) + } + return nil, nil +} + +func (f *fakeFileSystem) Open(name string) (*os.File, error) { + if f.openFn != nil { + return f.openFn(name) + } + return nil, nil +} + +func (f *fakeFileSystem) Remove(name string) error { + atomic.AddInt32(&f.removed, 1) + + if f.removeFn != nil { + return f.removeFn(name) + } + return nil +} + +func (f *fakeFileSystem) Removed() bool { + return atomic.LoadInt32(&f.removed) > 0 +}