hacky workaround: use locking to quiet race detector problems

So we have a problem which is triggered in part by the race detector,
but which is actually deeper, but also possibly rare enough to be
politely ignored.

The real underlying issue is that sometimes when we have multiple
tests running in CI, multiple instances of the CLI test end up using
the same postgres database backing for some of their DAX stuff. We
have workarounds for this in some places, but not others.

But the *observed symptom* of this is that it can cause a trivial
race detector issue where we have one call to `(*Resource).Lock()`
and another call to `(*Resource).IsLocked()` which aren't synchronized
in any way, so if the race detector spots this, it complains.

We can suppress that very easily by synchronizing these. That does
not solve the other possibly-weird problems, so this may not actually
address the issue, but I think it might reduce the rate of sporadic
failures significantly, which would give us some time to think about
solving the deeper problem.

The underlying design issue is that we're reusing the database name
in postgres for testing. This lets us have bounded growth (one database)
while leaving the database contents up after a failed test (so we can
examine them), then truncating the database during startup if it already
exists. Which works fine if *only one thing runs at once*, which would
be true on a laptop, but in CI, it's sometimes not true. A real fix
for that is complex and requires some rethinking of how we approach
the test stuff, as we don't want unbounded growth, but we also don't
want two copies of the test running at once to see each other, and
ensuring cleanup after a test failure is surprisingly hard.

Author: seebs

dax/storage/storage.go

@@ -243,6 +243,12 @@ type Resource struct {
 	latestWLVersion    int
 	lastWLPos          int
 
+	// temporary workaround: we use this to control access to locked
+	// because it can cause race detector failures in testing under
+	// circumstances. these circumstances are probably actually a
+	// different and more serious bug, but we want CI to run in the
+	// mean time.
+	mu     sync.Mutex
 	locked bool
 
 	dirty bool
@@ -259,6 +265,13 @@ func (m *Resource) initialize() *Resource {
 // believes it holds the lock. It does not look at the state of
 // underlying storage to verify the lock.
 func (m *Resource) IsLocked() bool {
+	// WARNING: This is probably wrong. The problem this immediately
+	// solves is race detector complaining about writes in Lock()
+	// racing against this. That's valid. But we shouldn't be getting
+	// there at all, so something else is also wrong. This is a
+	// WORKAROUND.
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	return m.locked
 }
 
@@ -373,6 +386,13 @@ func (m *Resource) Lock() error {
 	if err := m.writelogger.Lock(m.bucket, m.key); err != nil {
 		return errors.Wrap(err, "acquiring lock")
 	}
+	// WARNING: This is probably wrong. The problem this immediately
+	// solves is race detector complaining about writes in Lock()
+	// racing against this. That's valid. But we shouldn't be getting
+	// there at all, so something else is also wrong. This is a
+	// WORKAROUND.
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	m.locked = true
 	return nil
 }
@@ -453,6 +473,13 @@ func (m *Resource) Unlock() error {
 	if err := m.writelogger.Unlock(m.bucket, m.key); err != nil {
 		return errors.Wrap(err, "unlocking")
 	}
+	// WARNING: This is probably wrong. The problem this immediately
+	// solves is race detector complaining about writes in Lock()
+	// racing against this. That's valid. But we shouldn't be getting
+	// there at all, so something else is also wrong. This is a
+	// WORKAROUND.
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	m.locked = false
 	return nil
 }