Make DefaultRecorder goroutine safe

[codemental]

The recorder.DefaultRecorder is currently not goroutine safe. While this does not affect Clarum at this point, it is not nice to have and the fix should also be simple.

type DefaultRecorder struct {
    logResult *strings.Builder
}

func NewDefaultRecorder() Recorder {
    return &DefaultRecorder{
        logResult: new(strings.Builder),
    }
}

Goals

• DefaultRecorder is goroutine safe
• unit test that checks this
• documentation is updated: "it is not goroutine safe!" is removed

[robinmrtn]

I don't see how making this variable a pointer would help with threadsafety. This is a private variable only accessed via the method defined in the recorder interface. If you want to ensure the the order of the calls to WriteString() inside a multiple goroutines, you need to use a mutex lock.

Or am I missing something here? 🤔

[codemental]

In the current state a problem will occur only if we have tests that run in parallel (which we don't support at the moment). In such a scenario we can have multiple goroutines using the default strings.Builder and writing in parallel to the same underlying array. If we create a new Builder every time, we should not have this problem.

To answer your question, the new(...) function returns a pointer to that struct, so it requires the logResult to be a pointer.

[robinmrtn]

But a new string.Builder is already created with each call to the NewDefaultRecorder() constructor. I don't see that one strings.Builder writes to the same underlying byte array as another. https://go.dev/play/p/PzNJoYINMsT

I just see that some writes get ignored, which can be fixed by using Mutex.

[codemental]

I've checked your code and it is proof that the Recorder is coroutine safe. I remember reading somewhere that is it not, but maybe they had a different scenario. I've also checked the documentation again.

Anyway, with your example we now know that this issue does not exist. So you can close this ticket 🙂