From 2d2029c5982e898b39e826a5eee76d6107d12b50 Mon Sep 17 00:00:00 2001 From: limiting-factor Date: Sat, 4 Apr 2026 16:29:14 +0200 Subject: [PATCH] tests: make buffer log writer thread safe (#11962) When two goroutines attempt to access the content of the buffer log writer, they must be made thread safe with a write mutex. The buffer log writer is only used in testing. ## Checklist ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [ ] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11962 Reviewed-by: Mathieu Fenniak Co-authored-by: limiting-factor Co-committed-by: limiting-factor --- .deadcode-out | 3 ++ modules/log/event_writer_buffer.go | 42 ++++++++++++++++++++----- modules/log/event_writer_buffer_test.go | 12 +++---- modules/log/logger_impl_test.go | 2 +- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/.deadcode-out b/.deadcode-out index 45ad117ccd..0c44d2bdfd 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -134,6 +134,9 @@ forgejo.org/modules/json StdJSON.Indent forgejo.org/modules/log + eventWriterBuffer.Close + eventWriterBuffer.Write + eventWriterBuffer.GetString NewEventWriterBuffer forgejo.org/modules/markup diff --git a/modules/log/event_writer_buffer.go b/modules/log/event_writer_buffer.go index 28857c2189..a7557618a8 100644 --- a/modules/log/event_writer_buffer.go +++ b/modules/log/event_writer_buffer.go @@ -5,18 +5,44 @@ package log import ( "bytes" + "sync" ) -type EventWriterBuffer struct { - *EventWriterBaseImpl - Buffer *bytes.Buffer +type EventWriterBuffer interface { + EventWriter + GetString() string } -var _ EventWriter = (*EventWriterBuffer)(nil) +type eventWriterBuffer struct { + *EventWriterBaseImpl + buffer *bytes.Buffer + mu sync.RWMutex +} -func NewEventWriterBuffer(name string, mode WriterMode) *EventWriterBuffer { - w := &EventWriterBuffer{EventWriterBaseImpl: NewEventWriterBase(name, "buffer", mode)} - w.Buffer = new(bytes.Buffer) - w.OutputWriteCloser = nopCloser{w.Buffer} +var _ EventWriterBuffer = (*eventWriterBuffer)(nil) + +func (*eventWriterBuffer) Close() error { + return nil +} + +func (o *eventWriterBuffer) Write(p []byte) (n int, err error) { + o.mu.Lock() + defer o.mu.Unlock() + return o.buffer.Write(p) +} + +func (o *eventWriterBuffer) GetString() string { + o.mu.Lock() + defer o.mu.Unlock() + b := o.buffer.Bytes() + s := make([]byte, len(b)) + copy(s, b) + return string(s) +} + +func NewEventWriterBuffer(name string, mode WriterMode) EventWriter { + w := &eventWriterBuffer{EventWriterBaseImpl: NewEventWriterBase(name, "buffer", mode)} + w.buffer = new(bytes.Buffer) + w.OutputWriteCloser = w return w } diff --git a/modules/log/event_writer_buffer_test.go b/modules/log/event_writer_buffer_test.go index d1e37c3673..ac0759ad0b 100644 --- a/modules/log/event_writer_buffer_test.go +++ b/modules/log/event_writer_buffer_test.go @@ -29,7 +29,7 @@ func TestBufferLogger(t *testing.T) { MsgSimpleText: expected, }) logger.Close() - assert.Contains(t, bufferWriter.Buffer.String(), expected) + assert.Contains(t, bufferWriter.(log.EventWriterBuffer).GetString(), expected) } func TestBufferLoggerWithExclusion(t *testing.T) { @@ -41,7 +41,7 @@ func TestBufferLoggerWithExclusion(t *testing.T) { Level: level, Prefix: prefix, Exclusion: message, - }) + }).(log.EventWriterBuffer) logger := log.NewLoggerWithWriters(t.Context(), "test", bufferWriter) @@ -50,7 +50,7 @@ func TestBufferLoggerWithExclusion(t *testing.T) { MsgSimpleText: message, }) logger.Close() - assert.NotContains(t, bufferWriter.Buffer.String(), message) + assert.NotContains(t, bufferWriter.GetString(), message) } func TestBufferLoggerWithExpressionAndExclusion(t *testing.T) { @@ -64,7 +64,7 @@ func TestBufferLoggerWithExpressionAndExclusion(t *testing.T) { Prefix: prefix, Expression: expression, Exclusion: exclusion, - }) + }).(log.EventWriterBuffer) logger := log.NewLoggerWithWriters(t.Context(), "test", bufferWriter) @@ -74,6 +74,6 @@ func TestBufferLoggerWithExpressionAndExclusion(t *testing.T) { logger.SendLogEvent(&log.Event{Level: log.INFO, MsgSimpleText: "none"}) logger.Close() - assert.Contains(t, bufferWriter.Buffer.String(), "foo expression") - assert.NotContains(t, bufferWriter.Buffer.String(), "bar") + assert.Contains(t, bufferWriter.GetString(), "foo expression") + assert.NotContains(t, bufferWriter.GetString(), "bar") } diff --git a/modules/log/logger_impl_test.go b/modules/log/logger_impl_test.go index 59276a83f4..1d77ae0c25 100644 --- a/modules/log/logger_impl_test.go +++ b/modules/log/logger_impl_test.go @@ -23,5 +23,5 @@ func TestLog(t *testing.T) { testGeneric(logger, "I'm the generic value!") logger.Close() - assert.Contains(t, bufferWriter.Buffer.String(), ".../logger_impl_test.go:13:testGeneric() [I] Just testing the logging of a generic function I'm the generic value!") + assert.Contains(t, bufferWriter.(EventWriterBuffer).GetString(), ".../logger_impl_test.go:13:testGeneric() [I] Just testing the logging of a generic function I'm the generic value!") }