From 49cf16d3f2c9775233553b42e1e00b7e6a56dada Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Mon, 7 Aug 2023 23:14:31 +0200 Subject: [PATCH] fix race in test setup/teardown not easily triggered, but it happened just now on a build server. --- ctl_test.go | 3 +-- imapserver/fuzz_test.go | 3 +-- imapserver/server_test.go | 8 +++----- import.go | 3 +-- queue/queue_test.go | 4 ++-- serve.go | 3 ++- smtpserver/fuzz_test.go | 3 +-- smtpserver/server_test.go | 6 +++--- store/account_test.go | 3 +-- store/export_test.go | 3 +-- store/state.go | 14 +++++++++----- webaccount/account_test.go | 3 +-- webmail/api_test.go | 3 +-- webmail/view_test.go | 3 +-- webmail/webmail_test.go | 3 +-- 15 files changed, 29 insertions(+), 36 deletions(-) diff --git a/ctl_test.go b/ctl_test.go index 99509d8..4aa18ae 100644 --- a/ctl_test.go +++ b/ctl_test.go @@ -38,8 +38,7 @@ func TestCtl(t *testing.T) { if errs := mox.LoadConfig(ctxbg, true, false); len(errs) > 0 { t.Fatalf("loading mox config: %v", errs) } - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() xlog := mlog.New("ctl") diff --git a/imapserver/fuzz_test.go b/imapserver/fuzz_test.go index 515b3fc..512bdc1 100644 --- a/imapserver/fuzz_test.go +++ b/imapserver/fuzz_test.go @@ -71,8 +71,7 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("set password: %v", err) } - done := store.Switchboard() - defer close(done) + defer store.Switchboard()() comm := store.RegisterComm(acc) defer comm.Unregister() diff --git a/imapserver/server_test.go b/imapserver/server_test.go index 3d3ed09..c1ec8b0 100644 --- a/imapserver/server_test.go +++ b/imapserver/server_test.go @@ -346,11 +346,9 @@ func startArgs(t *testing.T, first, isTLS, allowLoginWithoutTLS bool) *testconn err = acc.SetPassword("testtest") tcheck(t, err, "set password") } - var switchDone chan struct{} + switchStop := func() {} if first { - switchDone = store.Switchboard() - } else { - switchDone = make(chan struct{}) // Dummy, that can be closed. + switchStop = store.Switchboard() } serverConn, clientConn := net.Pipe() @@ -368,7 +366,7 @@ func startArgs(t *testing.T, first, isTLS, allowLoginWithoutTLS bool) *testconn cid := connCounter go func() { serve("test", cid, tlsConfig, serverConn, isTLS, allowLoginWithoutTLS) - close(switchDone) + switchStop() close(done) }() client, err := imapclient.New(clientConn, true) diff --git a/import.go b/import.go index 4afe42e..84696e3 100644 --- a/import.go +++ b/import.go @@ -118,8 +118,7 @@ func xcmdXImport(mbox bool, c *cmd) { mox.Conf.Dynamic.Accounts = map[string]config.Account{ account: {}, } - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() xlog := mlog.New("import") cconn, sconn := net.Pipe() diff --git a/queue/queue_test.go b/queue/queue_test.go index 68c35d6..5960245 100644 --- a/queue/queue_test.go +++ b/queue/queue_test.go @@ -45,14 +45,14 @@ func setup(t *testing.T) (*store.Account, func()) { tcheck(t, err, "open account") err = acc.SetPassword("testtest") tcheck(t, err, "set password") - switchDone := store.Switchboard() + switchStop := store.Switchboard() mox.Shutdown, mox.ShutdownCancel = context.WithCancel(ctxbg) return acc, func() { acc.Close() mox.ShutdownCancel() mox.Shutdown, mox.ShutdownCancel = context.WithCancel(ctxbg) Shutdown() - close(switchDone) + switchStop() } } diff --git a/serve.go b/serve.go index 2554d59..55327a9 100644 --- a/serve.go +++ b/serve.go @@ -624,7 +624,8 @@ func start(mtastsdbRefresher, skipForkExec bool) error { http.Serve() go func() { - <-store.Switchboard() + store.Switchboard() + <-make(chan struct{}) }() return nil } diff --git a/smtpserver/fuzz_test.go b/smtpserver/fuzz_test.go index 3e99160..644d825 100644 --- a/smtpserver/fuzz_test.go +++ b/smtpserver/fuzz_test.go @@ -43,8 +43,7 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("set password: %v", err) } - done := store.Switchboard() - defer close(done) + defer store.Switchboard()() err = queue.Init() if err != nil { f.Fatalf("queue init: %v", err) diff --git a/smtpserver/server_test.go b/smtpserver/server_test.go index 6a8249c..5949e18 100644 --- a/smtpserver/server_test.go +++ b/smtpserver/server_test.go @@ -75,7 +75,7 @@ test email type testserver struct { t *testing.T acc *store.Account - switchDone chan struct{} + switchStop func() comm *store.Comm cid int64 resolver dns.Resolver @@ -101,7 +101,7 @@ func newTestServer(t *testing.T, configPath string, resolver dns.Resolver) *test tcheck(t, err, "open account") err = ts.acc.SetPassword("testtest") tcheck(t, err, "set password") - ts.switchDone = store.Switchboard() + ts.switchStop = store.Switchboard() err = queue.Init() tcheck(t, err, "queue init") @@ -116,7 +116,7 @@ func (ts *testserver) close() { } ts.comm.Unregister() queue.Shutdown() - close(ts.switchDone) + ts.switchStop() err := ts.acc.Close() tcheck(ts.t, err, "closing account") ts.acc = nil diff --git a/store/account_test.go b/store/account_test.go index b96ba15..cd151a0 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -36,8 +36,7 @@ func TestMailbox(t *testing.T) { err = acc.Close() tcheck(t, err, "closing account") }() - switchDone := Switchboard() - defer close(switchDone) + defer Switchboard()() log := mlog.New("store") diff --git a/store/export_test.go b/store/export_test.go index 16da834..cb44b8e 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -25,8 +25,7 @@ func TestExport(t *testing.T) { acc, err := OpenAccount("mjl") tcheck(t, err, "open account") defer acc.Close() - switchDone := Switchboard() - defer close(switchDone) + defer Switchboard()() log := mlog.New("export") diff --git a/store/state.go b/store/state.go index c4b0d82..817f157 100644 --- a/store/state.go +++ b/store/state.go @@ -101,7 +101,7 @@ type ChangeMailboxKeywords struct { var switchboardBusy atomic.Bool // Switchboard distributes changes to accounts to interested listeners. See Comm and Change. -func Switchboard() chan struct{} { +func Switchboard() (stop func()) { regs := map[*Account]map[*Comm]struct{}{} done := make(chan struct{}) @@ -145,14 +145,18 @@ func Switchboard() chan struct{} { chReq.done <- struct{}{} case <-done: - if !switchboardBusy.CompareAndSwap(true, false) { - panic("switchboard already unregistered?") - } + done <- struct{}{} return } } }() - return done + return func() { + done <- struct{}{} + <-done + if !switchboardBusy.CompareAndSwap(true, false) { + panic("switchboard already unregistered?") + } + } } // Comm handles communication with the goroutine that maintains the diff --git a/webaccount/account_test.go b/webaccount/account_test.go index 41993a6..690e740 100644 --- a/webaccount/account_test.go +++ b/webaccount/account_test.go @@ -46,8 +46,7 @@ func TestAccount(t *testing.T) { err = acc.Close() tcheck(t, err, "closing account") }() - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() log := mlog.New("store") diff --git a/webmail/api_test.go b/webmail/api_test.go index 5f9a9b5..cfa59ca 100644 --- a/webmail/api_test.go +++ b/webmail/api_test.go @@ -45,8 +45,7 @@ func TestAPI(t *testing.T) { mox.Context = ctxbg mox.ConfigStaticPath = "../testdata/webmail/mox.conf" mox.MustLoadConfig(true, false) - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() acc, err := store.OpenAccount("mjl") tcheck(t, err, "open account") diff --git a/webmail/view_test.go b/webmail/view_test.go index b0ee2d4..63d2463 100644 --- a/webmail/view_test.go +++ b/webmail/view_test.go @@ -25,8 +25,7 @@ func TestView(t *testing.T) { mox.Context = ctxbg mox.ConfigStaticPath = "../testdata/webmail/mox.conf" mox.MustLoadConfig(true, false) - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() acc, err := store.OpenAccount("mjl") tcheck(t, err, "open account") diff --git a/webmail/webmail_test.go b/webmail/webmail_test.go index 58204ca..cbba192 100644 --- a/webmail/webmail_test.go +++ b/webmail/webmail_test.go @@ -266,8 +266,7 @@ func TestWebmail(t *testing.T) { mox.Context = ctxbg mox.ConfigStaticPath = "../testdata/webmail/mox.conf" mox.MustLoadConfig(true, false) - switchDone := store.Switchboard() - defer close(switchDone) + defer store.Switchboard()() acc, err := store.OpenAccount("mjl") tcheck(t, err, "open account")