From 283b00c741d1cfa454fcdfbc40bc0e90170da2c0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 27 Mar 2023 14:39:38 -0500 Subject: [PATCH] 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. --- dax/storage/storage.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/dax/storage/storage.go b/dax/storage/storage.go index cdc72fa32..0808ee055 100644 --- a/dax/storage/storage.go +++ b/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 }