fix(logger): make MockLogger lock the buffer implicitly

cmd/snap-bootstrap/cmd_initramfs_mounts_test.go

@@ -67,7 +67,7 @@ type baseInitramfsMountsSuite struct {
 	*seedtest.TestingSeed20
 
 	Stdout *bytes.Buffer
-	logs   *bytes.Buffer
+	logs   logger.MockedLogger
 
 	seedDir    string
 	byLabelDir string

cmd/snap-failure/main_test.go

@@ -42,7 +42,7 @@ type failureSuite struct {
 
 	stderr *bytes.Buffer
 	stdout *bytes.Buffer
-	log    *bytes.Buffer
+	log    logger.MockedLogger
 
 	systemctlCmd *testutil.MockCmd
 }

cmd/snap-update-ns/main_test.go

@@ -20,7 +20,6 @@
 package main_test
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -42,7 +41,7 @@ func Test(t *testing.T) { TestingT(t) }
 type mainSuite struct {
 	testutil.BaseTest
 	as  *update.Assumptions
-	log *bytes.Buffer
+	log logger.MockedLogger
 }
 
 var _ = Suite(&mainSuite{})

cmd/snap-update-ns/update_test.go

@@ -20,7 +20,6 @@
 package main_test
 
 import (
-	"bytes"
 	"fmt"
 	"path/filepath"
 	"syscall"
@@ -36,7 +35,7 @@ import (
 
 type updateSuite struct {
 	testutil.BaseTest
-	log *bytes.Buffer
+	log logger.MockedLogger
 }
 
 var _ = Suite(&updateSuite{})

cmd/snap-update-ns/utils_test.go

@@ -20,7 +20,6 @@
 package main_test
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"io/fs"
@@ -40,7 +39,7 @@ import (
 type utilsSuite struct {
 	testutil.BaseTest
 	sys *testutil.SyscallRecorder
-	log *bytes.Buffer
+	log logger.MockedLogger
 	as  *update.Assumptions
 }
 

cmd/snapd-apparmor/main_test.go

@@ -20,7 +20,6 @@
 package main_test
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -235,7 +234,7 @@ func (s *mainSuite) TestValidateArgs(c *C) {
 type integrationSuite struct {
 	testutil.BaseTest
 
-	logBuf    *bytes.Buffer
+	logBuf    logger.MockedLogger
 	parserCmd *testutil.MockCmd
 }
 

httputil/client_test.go

@@ -20,7 +20,6 @@
 package httputil_test
 
 import (
-	"bytes"
 	"crypto/rand"
 	"crypto/rsa"
 	"crypto/tls"
@@ -151,7 +150,7 @@ type tlsSuite struct {
 
 	tmpdir            string
 	certpath, keypath string
-	logbuf            *bytes.Buffer
+	logbuf            logger.MockedLogger
 
 	srv *httptest.Server
 }

httputil/logger_test.go

@@ -39,7 +39,7 @@ import (
 func TestHTTPUtil(t *testing.T) { check.TestingT(t) }
 
 type loggerSuite struct {
-	logbuf        *bytes.Buffer
+	logbuf        logger.MockedLogger
 	restoreLogger func()
 }
 

logger/logger.go

@@ -117,10 +117,39 @@ func NoGuardDebugf(format string, v ...interface{}) {
 	logger.NoGuardDebug(msg)
 }
 
+type mockedLogger struct {
+	buffer bytes.Buffer
+	mutex  sync.Mutex
+}
+
+func (b *mockedLogger) Write(p []byte) (int, error) {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	return b.buffer.Write(p)
+}
+
+func (b *mockedLogger) String() string {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	return b.buffer.String()
+}
+
+func (b *mockedLogger) Reset() {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	b.buffer.Reset()
+}
+
+type MockedLogger interface {
+	fmt.Stringer
+
+	Reset()
+}
+
 // MockLogger replaces the existing logger with a buffer and returns
-// the log buffer and a restore function.
-func MockLogger() (buf *bytes.Buffer, restore func()) {
-	buf = &bytes.Buffer{}
+// a MockedLogger (with String() and Reset()) and a restore function.
+func MockLogger() (MockedLogger, func()) {
+	buf := &mockedLogger{}
 	oldLogger := logger
 	l, err := New(buf, DefaultFlags, &LoggerOptions{})
 	if err != nil {
@@ -132,15 +161,6 @@ func MockLogger() (buf *bytes.Buffer, restore func()) {
 	}
 }
 
-// WithLoggerLock invokes f with the global logger lock, useful for
-// tests involving goroutines with MockLogger.
-func WithLoggerLock(f func()) {
-	lock.Lock()
-	defer lock.Unlock()
-
-	f()
-}
-
 // SetLogger sets the global logger to the given one
 func SetLogger(l Logger) {
 	lock.Lock()

logger/logger_test.go

@@ -32,6 +32,8 @@
 
 	. "gopkg.in/check.v1"
 
+	"gopkg.in/tomb.v2"
+
 	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/osutil/kcmdline"
 	"github.com/snapcore/snapd/testutil"
@@ -44,7 +46,7 @@
 
 type LogSuite struct {
 	testutil.BaseTest
-	logbuf        *bytes.Buffer
+	logbuf        logger.MockedLogger
 	restoreLogger func()
 }
 
@@ -163,15 +165,20 @@
 	c.Check(s.logbuf.String(), Matches, `(?m).*logger_test\.go:\d+: PANIC xyzzy`)
 }
 
-func (s *LogSuite) TestWithLoggerLock(c *C) {
-	logger.Noticef("xyzzy")
-
-	called := false
-	logger.WithLoggerLock(func() {
-		called = true
-		c.Check(s.logbuf.String(), Matches, `(?m).*logger_test\.go:\d+: xyzzy`)
+func (s *LogSuite) TestMockLoggerReadWriteThreadsafe(c *C) {
+	var t tomb.Tomb
+	t.Go(func() error {
+		for i := 0; i < 100; i++ {
+			logger.Noticef("foo")
+			logger.Noticef("bar")
+		}
+		return nil
 	})
-	c.Check(called, Equals, true)
+	for i := 0; i < 10; i++ {
+		logger.Noticef(s.logbuf.String())
+	}
+	err := t.Wait()
+	c.Check(err, IsNil)
 }
 
 func (s *LogSuite) TestNoGuardDebug(c *C) {

overlord/devicestate/devicestate_cloudinit_test.go

@@ -1,7 +1,6 @@
 package devicestate_test
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -22,7 +21,7 @@ import (
 
 type cloudInitBaseSuite struct {
 	deviceMgrBaseSuite
-	logbuf *bytes.Buffer
+	logbuf logger.MockedLogger
 }
 
 type cloudInitSuite struct {

overlord/devicestate/devicestate_systems_test.go

@@ -72,7 +72,7 @@ type mockedSystemSeed struct {
 type deviceMgrSystemsBaseSuite struct {
 	deviceMgrBaseSuite
 
-	logbuf            *bytes.Buffer
+	logbuf            logger.MockedLogger
 	mockedSystemSeeds []mockedSystemSeed
 	ss                *seedtest.SeedSnaps
 	model             *asserts.Model

overlord/devicestate/systems_test.go

@@ -20,7 +20,6 @@
 package devicestate_test
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -47,7 +46,7 @@ type createSystemSuite struct {
 
 	ss *seedtest.SeedSnaps
 
-	logbuf *bytes.Buffer
+	logbuf logger.MockedLogger
 }
 
 var _ = Suite(&createSystemSuite{})

overlord/ifacestate/apparmorprompting/prompting_test.go

@@ -214,10 +214,8 @@ func (s *apparmorpromptingSuite) TestHandleListenerRequestErrors(c *C) {
 	resp, err := waitForReply(replyChan)
 	c.Assert(err, IsNil)
 	c.Check(resp.Request, Equals, req)
-	logger.WithLoggerLock(func() {
-		c.Check(logbuf.String(), testutil.Contains,
-			` error while parsing AppArmor permissions: cannot get abstract permissions from empty AppArmor permissions: "none"`)
-	})
+	c.Check(logbuf.String(), testutil.Contains,
+		` error while parsing AppArmor permissions: cannot get abstract permissions from empty AppArmor permissions: "none"`)
 
 	// Fill the requestprompts backend until we hit its outstanding prompt
 	// count limit
@@ -238,9 +236,7 @@ func (s *apparmorpromptingSuite) TestHandleListenerRequestErrors(c *C) {
 	c.Assert(len(prompts), Equals, maxOutstandingPromptsPerUser)
 
 	// Now try to add one more request, it should fail
-	logger.WithLoggerLock(func() {
-		logbuf.Reset()
-	})
+	logbuf.Reset()
 
 	req = &listener.Request{
 		Label:      "snap.firefox.firefox",
@@ -251,10 +247,8 @@ func (s *apparmorpromptingSuite) TestHandleListenerRequestErrors(c *C) {
 	}
 	reqChan <- req
 	time.Sleep(10 * time.Millisecond)
-	logger.WithLoggerLock(func() {
-		c.Check(logbuf.String(), testutil.Contains,
-			" WARNING: too many outstanding prompts for user 1000; auto-denying new one\n")
-	})
+	c.Check(logbuf.String(), testutil.Contains,
+		" WARNING: too many outstanding prompts for user 1000; auto-denying new one\n")
 
 	c.Assert(mgr.Stop(), IsNil)
 }
@@ -308,7 +302,7 @@ func (s *apparmorpromptingSuite) simulateRequest(c *C, reqChan chan *listener.Re
 
 	// Check that no error occurred
 	time.Sleep(10 * time.Millisecond)
-	logger.WithLoggerLock(func() { c.Assert(logbuf.String(), Equals, "") })
+	c.Assert(logbuf.String(), Equals, "")
 
 	// which should generate a notice
 	s.st.Lock()

overlord/ifacestate/ifacestate_test.go

@@ -20,7 +20,6 @@
 package ifacestate_test
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -180,7 +179,7 @@ type interfaceManagerSuite struct {
 	extraBackends  []interfaces.SecurityBackend
 	secBackend     *ifacetest.TestSecurityBackend
 	mockSnapCmd    *testutil.MockCmd
-	log            *bytes.Buffer
+	log            logger.MockedLogger
 	coreSnap       *interfaces.SnapAppSet
 	snapdSnap      *interfaces.SnapAppSet
 
@@ -7010,9 +7009,7 @@ func (s *interfaceManagerSuite) TestInitInterfacesRequestsManagerError(c *C) {
 	running := mgr.AppArmorPromptingRunning()
 	c.Check(running, Equals, false)
 
-	logger.WithLoggerLock(func() {
-		c.Check(logbuf.String(), testutil.Contains, fmt.Sprintf("%v", createError))
-	})
+	c.Check(logbuf.String(), testutil.Contains, fmt.Sprintf("%v", createError))
 }
 
 func (s *interfaceManagerSuite) TestStopInterfacesRequestsManagerError(c *C) {

overlord/managers_test.go

@@ -148,7 +148,7 @@ type baseMgrsSuite struct {
 
 	automaticSnapshots []automaticSnapshotCall
 
-	logbuf *bytes.Buffer
+	logbuf logger.MockedLogger
 
 	storeObserver func(r *http.Request)
 }

overlord/restart/restart_test.go

@@ -20,7 +20,6 @@
 package restart_test
 
 import (
-	"bytes"
 	"errors"
 	"path/filepath"
 	"testing"
@@ -1183,7 +1182,7 @@ type notifyRebootRequiredSuite struct {
 
 	st          *state.State
 	mockNrrPath string
-	mockLog     *bytes.Buffer
+	mockLog     logger.MockedLogger
 	chg         *state.Change
 	t1          *state.Task
 }

snapdtool/fips_linux_test.go

@@ -1,7 +1,6 @@
 package snapdtool_test
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -20,7 +19,7 @@ import (
 type fipsSuite struct {
 	testutil.BaseTest
 
-	logbuf *bytes.Buffer
+	logbuf logger.MockedLogger
 }
 
 var _ = Suite(&fipsSuite{})

store/store_test.go

@@ -180,7 +180,7 @@ type baseStoreSuite struct {
 
 	ctx context.Context
 
-	logbuf *bytes.Buffer
+	logbuf logger.MockedLogger
 }
 
 const (