From 14b3d80bc57fee87ea167401c6e638b066de40dd Mon Sep 17 00:00:00 2001 From: Abdelrahman Abdelraouf Date: Tue, 29 Jul 2025 18:00:07 +0100 Subject: [PATCH 1/2] :bug: `logs` Fix missing writer close skip --- changes/20250729175928.bugfix | 1 + utils/logs/json_logger.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 changes/20250729175928.bugfix diff --git a/changes/20250729175928.bugfix b/changes/20250729175928.bugfix new file mode 100644 index 0000000000..3fab72f12d --- /dev/null +++ b/changes/20250729175928.bugfix @@ -0,0 +1 @@ +:bug: `logs` Fix missing writer close skip diff --git a/utils/logs/json_logger.go b/utils/logs/json_logger.go index 4a09533674..0d02a9f427 100644 --- a/utils/logs/json_logger.go +++ b/utils/logs/json_logger.go @@ -45,6 +45,7 @@ func (l *JSONLoggers) GetSource() string { defer l.mu.RUnlock() return l.source } + func (l *JSONLoggers) GetLoggerSource() string { l.mu.RLock() defer l.mu.RUnlock() @@ -156,5 +157,5 @@ func NewJSONLoggerForSlowWriter(slowWriter WriterWithSource, ringBufferSize int, // droppedMessagesLogger : logger for dropped messages // If pollInterval is greater than 0, a poller is used otherwise a waiter is used. func NewJSONLoggerForSlowWriterWithoutClosingWriter(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, loggerSource string, source string, droppedMessagesLogger Loggers) (loggers Loggers, err error) { - return NewJSONLogger(NewDiodeWriterForSlowWriterWithoutClosing(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) + return NewJSONLoggerWithWriter(NewDiodeWriterForSlowWriterWithoutClosing(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) } From cbf5853d3c9acc68d503abfb753fdb8e8fbd1608 Mon Sep 17 00:00:00 2001 From: Abdelrahman Abdelraouf Date: Wed, 30 Jul 2025 09:32:38 +0100 Subject: [PATCH 2/2] Fix --- utils/logs/json_logger.go | 13 +++++++++---- utils/logs/json_logger_test.go | 7 ++----- utils/logs/log_test.go | 4 +--- utils/logs/writer.go | 13 ++++--------- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/utils/logs/json_logger.go b/utils/logs/json_logger.go index 0d02a9f427..04aee1f24e 100644 --- a/utils/logs/json_logger.go +++ b/utils/logs/json_logger.go @@ -72,7 +72,7 @@ func (l *JSONLoggers) Configure() error { } // Log logs to the output stream. -func (l *JSONLoggers) Log(output ...interface{}) { +func (l *JSONLoggers) Log(output ...any) { if len(output) == 1 && output[0] == "\n" { return } @@ -80,7 +80,7 @@ func (l *JSONLoggers) Log(output ...interface{}) { } // LogError logs to the error stream. -func (l *JSONLoggers) LogError(err ...interface{}) { +func (l *JSONLoggers) LogError(err ...any) { if len(err) == 1 && err[0] == "\n" { return } @@ -99,7 +99,7 @@ func NewJSONLogger(writer WriterWithSource, loggerSource string, source string) return newJSONLogger(true, writer, loggerSource, source) } -// NewJSONLogger creates a Json logger. It is similar to NewJSONLogger but does not close the writer on Close(). +// NewJSONLoggerWithWriter creates a Json logger. It is similar to NewJSONLogger but does not close the writer on Close(). func NewJSONLoggerWithWriter(writer WriterWithSource, loggerSource string, source string) (Loggers, error) { return newJSONLogger(false, writer, loggerSource, source) } @@ -157,5 +157,10 @@ func NewJSONLoggerForSlowWriter(slowWriter WriterWithSource, ringBufferSize int, // droppedMessagesLogger : logger for dropped messages // If pollInterval is greater than 0, a poller is used otherwise a waiter is used. func NewJSONLoggerForSlowWriterWithoutClosingWriter(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, loggerSource string, source string, droppedMessagesLogger Loggers) (loggers Loggers, err error) { - return NewJSONLoggerWithWriter(NewDiodeWriterForSlowWriterWithoutClosing(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) + // We call NewJSONLogger which closes the writer, because in this case the writer is a DiodeWriter that doesn't close the writer that is passed to it. + // JSONLogger.Close() + // -> DiodeWriterWithoutClosing.Close() + // -> does the proper steps for closing it's own goroutines + // -> does not close the writer passed as it's passed a non-closeable writer + return NewJSONLogger(NewDiodeWriterForSlowWriterWithoutClosing(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) } diff --git a/utils/logs/json_logger_test.go b/utils/logs/json_logger_test.go index b9e7cfbd93..deedb545ea 100644 --- a/utils/logs/json_logger_test.go +++ b/utils/logs/json_logger_test.go @@ -21,11 +21,10 @@ func TestLogMessage(t *testing.T) { }) t.Run("without writer closing", func(t *testing.T) { writer := NewStdWriterWithSource() - defer func() { _ = writer.Close() }() + defer func() { require.NoError(t, writer.Close()) }() loggers, err := NewJSONLoggerWithWriter(writer, "Test", "TestLogMessage") require.NoError(t, err) testLog(t, loggers) - require.NoError(t, writer.Close()) }) } @@ -34,7 +33,6 @@ func TestLogMessageToSlowLogger(t *testing.T) { stdloggers, err := NewStdLogger("ERR:") require.NoError(t, err) - defer goleak.VerifyNone(t) t.Run("with writer closing", func(t *testing.T) { loggers, err := NewJSONLoggerForSlowWriter(NewTestSlowWriter(t), 1024, 2*time.Millisecond, "Test", "TestLogMessageToSlowLogger", stdloggers) require.NoError(t, err) @@ -43,11 +41,10 @@ func TestLogMessageToSlowLogger(t *testing.T) { }) t.Run("without writer closing", func(t *testing.T) { writer := NewTestSlowWriter(t) - defer func() { _ = writer.Close() }() + defer func() { require.NoError(t, writer.Close()) }() loggers, err := NewJSONLoggerForSlowWriterWithoutClosingWriter(writer, 1024, 2*time.Millisecond, "Test", "TestLogMessageToSlowLogger", stdloggers) require.NoError(t, err) testLog(t, loggers) time.Sleep(100 * time.Millisecond) - require.NoError(t, writer.Close()) }) } diff --git a/utils/logs/log_test.go b/utils/logs/log_test.go index 171f9ddf5a..2d502e61f6 100644 --- a/utils/logs/log_test.go +++ b/utils/logs/log_test.go @@ -27,7 +27,7 @@ func testLog(t *testing.T, loggers Loggers) { t.Helper() err := loggers.Check() require.NoError(t, err) - defer func() { _ = loggers.Close() }() + defer func() { require.NoError(t, loggers.Close()) }() err = loggers.SetLogSource("source1") require.NoError(t, err) @@ -60,6 +60,4 @@ func testLog(t *testing.T, loggers Loggers) { loggers.LogError(commonerrors.ErrUnexpected, "some error") loggers.LogError("some error", commonerrors.ErrUnexpected) loggers.LogError(nil, "no error") - err = loggers.Close() - require.NoError(t, err) } diff --git a/utils/logs/writer.go b/utils/logs/writer.go index c5f4e64264..cbfb9a1b1f 100644 --- a/utils/logs/writer.go +++ b/utils/logs/writer.go @@ -110,14 +110,10 @@ func (w *DiodeWriter) Write(p []byte) (n int, err error) { } func (w *DiodeWriter) Close() error { - err := w.slowWriter.Close() - if err != nil { - return err - } - if diodeCloser, ok := w.diodeWriter.(io.Closer); ok { - return diodeCloser.Close() + if w.closeStore == nil { + return nil } - return err + return w.closeStore.Close() } func (w *DiodeWriter) SetSource(source string) error { @@ -150,8 +146,7 @@ func newDiodeWriterForSlowWriter(closeWriterOnClose bool, slowWriter WriterWithS // We pass a wrapper that "overrides" the close method to do nothing, // this is because we control the writer's close behaviour through the `closeWriterOnClose` check - ncw := nonCloseableWriter{slowWriter} - d := diode.NewWriter(ncw, ringBufferSize, pollInterval, func(missed int) { + d := diode.NewWriter(nonCloseableWriter{Writer: slowWriter}, ringBufferSize, pollInterval, func(missed int) { if droppedMessagesLogger != nil { droppedMessagesLogger.LogError(fmt.Sprintf("Logger dropped %d messages", missed)) }