From c4255a96f819be2ccad326f6d40af7d28aeaf0a1 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sat, 15 Mar 2025 11:15:23 +0100 Subject: [PATCH] In tests, make initializing store/, its switchboard and an account more consistent. Initialize store and switchboard first, then open account, and close in reverse order. Replace all "CheckClosed" calls with "WaitClosed", future changings will keep an account reference open for a bit after the last regular close, so we can't know that an account should be closed during tests. Remove one parameter from the (still too long) "start test server" function in imapserver testing code. --- ctl_test.go | 13 ++++++------- imapserver/authenticate_test.go | 18 +++++++++--------- imapserver/fuzz_test.go | 3 ++- imapserver/server_test.go | 16 ++++++---------- queue/queue_test.go | 6 ++++-- smtpserver/alias_test.go | 2 +- smtpserver/fuzz_test.go | 3 ++- smtpserver/server_test.go | 16 ++++++++-------- store/account.go | 12 ++---------- store/account_test.go | 18 ++++++++++++++++-- store/export_test.go | 4 ++-- store/threads_test.go | 7 ++++--- webaccount/account_test.go | 5 +++-- webmail/api_test.go | 2 +- webmail/view_test.go | 2 +- webmail/webmail_test.go | 4 ++-- 16 files changed, 69 insertions(+), 62 deletions(-) diff --git a/ctl_test.go b/ctl_test.go index 48be61d..bf5f1c4 100644 --- a/ctl_test.go +++ b/ctl_test.go @@ -49,15 +49,14 @@ func TestCtl(t *testing.T) { if errs := mox.LoadConfig(ctxbg, pkglog, true, false); len(errs) > 0 { t.Fatalf("loading mox config: %v", errs) } - defer store.Switchboard()() - - err := queue.Init() - tcheck(t, err, "queue init") - defer queue.Shutdown() - - err = store.Init(ctxbg) + err := store.Init(ctxbg) tcheck(t, err, "store init") defer store.Close() + defer store.Switchboard()() + + err = queue.Init() + tcheck(t, err, "queue init") + defer queue.Shutdown() var cid int64 diff --git a/imapserver/authenticate_test.go b/imapserver/authenticate_test.go index c05b5d7..e69b86b 100644 --- a/imapserver/authenticate_test.go +++ b/imapserver/authenticate_test.go @@ -240,7 +240,7 @@ func TestAuthenticateCRAMMD5(t *testing.T) { } func TestAuthenticateTLSClientCert(t *testing.T) { - tc := startArgs(t, true, true, true, true, "mjl") + tc := startArgsMore(t, true, true, nil, nil, true, true, "mjl", nil) tc.transactf("no", "authenticate external ") // No TLS auth. tc.close() @@ -263,7 +263,7 @@ func TestAuthenticateTLSClientCert(t *testing.T) { } // No preauth, explicit authenticate with TLS. - tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, "mjl", addClientCert) if tc.client.Preauth { t.Fatalf("preauthentication while not configured for tls public key") } @@ -271,7 +271,7 @@ func TestAuthenticateTLSClientCert(t *testing.T) { tc.close() // External with explicit username. - tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, "mjl", addClientCert) if tc.client.Preauth { t.Fatalf("preauthentication while not configured for tls public key") } @@ -279,12 +279,12 @@ func TestAuthenticateTLSClientCert(t *testing.T) { tc.close() // No preauth, also allow other mechanisms. - tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, "mjl", addClientCert) tc.transactf("ok", "authenticate plain %s", base64.StdEncoding.EncodeToString([]byte("\u0000mjl@mox.example\u0000"+password0))) tc.close() // No preauth, also allow other username for same account. - tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, "mjl", addClientCert) tc.transactf("ok", "authenticate plain %s", base64.StdEncoding.EncodeToString([]byte("\u0000móx@mox.example\u0000"+password0))) tc.close() @@ -295,12 +295,12 @@ func TestAuthenticateTLSClientCert(t *testing.T) { tcheck(t, err, "set password") err = acc.Close() tcheck(t, err, "close account") - tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, nil, &clientConfig, false, true, "mjl", addClientCert) tc.transactf("no", "authenticate plain %s", base64.StdEncoding.EncodeToString([]byte("\u0000other@mox.example\u0000test1234"))) tc.close() // Starttls and external auth. - tc = startArgsMore(t, true, false, nil, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, false, nil, &clientConfig, false, true, "mjl", addClientCert) tc.client.Starttls(&clientConfig) tc.transactf("ok", "authenticate external =") tc.close() @@ -318,7 +318,7 @@ func TestAuthenticateTLSClientCert(t *testing.T) { defer cancel() mox.StartTLSSessionTicketKeyRefresher(ctx, pkglog, &serverConfig) clientConfig.ClientSessionCache = tls.NewLRUClientSessionCache(10) - tc = startArgsMore(t, true, true, &serverConfig, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, &serverConfig, &clientConfig, false, true, "mjl", addClientCert) if !tc.client.Preauth { t.Fatalf("not preauthentication while configured for tls public key") } @@ -330,7 +330,7 @@ func TestAuthenticateTLSClientCert(t *testing.T) { tc.close() // Authentication works with TLS resumption. - tc = startArgsMore(t, true, true, &serverConfig, &clientConfig, false, true, true, "mjl", addClientCert) + tc = startArgsMore(t, true, true, &serverConfig, &clientConfig, false, true, "mjl", addClientCert) if !tc.client.Preauth { t.Fatalf("not preauthentication while configured for tls public key") } diff --git a/imapserver/fuzz_test.go b/imapserver/fuzz_test.go index 66353e4..20f25b1 100644 --- a/imapserver/fuzz_test.go +++ b/imapserver/fuzz_test.go @@ -69,6 +69,8 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("store init: %v", err) } + defer store.Switchboard()() + acc, err := store.OpenAccount(log, "mjl", false) if err != nil { f.Fatalf("open account: %v", err) @@ -81,7 +83,6 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("set password: %v", err) } - defer store.Switchboard()() comm := store.RegisterComm(acc) defer comm.Unregister() diff --git a/imapserver/server_test.go b/imapserver/server_test.go index b6f5e9e..794e681 100644 --- a/imapserver/server_test.go +++ b/imapserver/server_test.go @@ -338,7 +338,6 @@ func (tc *testconn) close0(waitclose bool) { if waitclose { tc.account.WaitClosed() } - // no account.CheckClosed(), the tests open accounts multiple times. tc.account = nil tc.serverConn.Close() tc.waitDone() @@ -385,7 +384,7 @@ const password0 = "te\u0301st \u00a0\u2002\u200a" // NFD and various unicode spa const password1 = "tést " // PRECIS normalized, with NFC. func startArgs(t *testing.T, first, immediateTLS bool, allowLoginWithoutTLS, setPassword bool, accname string) *testconn { - return startArgsMore(t, first, immediateTLS, nil, nil, allowLoginWithoutTLS, false, setPassword, accname, nil) + return startArgsMore(t, first, immediateTLS, nil, nil, allowLoginWithoutTLS, setPassword, accname, nil) } // namedConn wraps a conn so it can return a RemoteAddr with a non-empty name. @@ -400,9 +399,10 @@ func (c namedConn) RemoteAddr() net.Addr { } // todo: the parameters and usage are too much now. change to scheme similar to smtpserver, with params in a struct, and a separate method for init and making a connection. -func startArgsMore(t *testing.T, first, immediateTLS bool, serverConfig, clientConfig *tls.Config, allowLoginWithoutTLS, noCloseSwitchboard, setPassword bool, accname string, afterInit func() error) *testconn { +func startArgsMore(t *testing.T, first, immediateTLS bool, serverConfig, clientConfig *tls.Config, allowLoginWithoutTLS, setPassword bool, accname string, afterInit func() error) *testconn { limitersInit() // Reset rate limiters. + switchStop := func() {} if first { mox.ConfigStaticPath = filepath.FromSlash("../testdata/imap/mox.conf") mox.MustLoadConfig(true, false) @@ -410,17 +410,16 @@ func startArgsMore(t *testing.T, first, immediateTLS bool, serverConfig, clientC os.RemoveAll("../testdata/imap/data") err := store.Init(ctxbg) tcheck(t, err, "store init") + switchStop = store.Switchboard() } + acc, err := store.OpenAccount(pkglog, accname, false) tcheck(t, err, "open account") if setPassword { err = acc.SetPassword(pkglog, password0) tcheck(t, err, "set password") } - switchStop := func() {} if first { - switchStop = store.Switchboard() - // Add a deleted mailbox, may excercise some code paths. err = acc.DB.Write(ctxbg, func(tx *bstore.Tx) error { // todo: add a message to inbox and remove it again. need to change all uids in the tests. @@ -487,15 +486,12 @@ func startArgsMore(t *testing.T, first, immediateTLS bool, serverConfig, clientC go func() { const viaHTTPS = false serve("test", cid, serverConfig, serverConn, immediateTLS, allowLoginWithoutTLS, viaHTTPS, "") - if !noCloseSwitchboard { - switchStop() - } close(done) }() client, err := imapclient.New(connCounter, clientConn, true) tcheck(t, err, "new client") tc := &testconn{t: t, conn: clientConn, client: client, done: done, serverConn: serverConn, account: acc} - if first && noCloseSwitchboard { + if first { tc.switchStop = switchStop } return tc diff --git a/queue/queue_test.go b/queue/queue_test.go index 4443c33..a8dc3f4 100644 --- a/queue/queue_test.go +++ b/queue/queue_test.go @@ -61,18 +61,20 @@ func setup(t *testing.T) (*store.Account, func()) { mox.Context = ctxbg mox.ConfigStaticPath = filepath.FromSlash("../testdata/queue/mox.conf") mox.MustLoadConfig(true, false) + mox.Shutdown, mox.ShutdownCancel = context.WithCancel(ctxbg) err := Init() tcheck(t, err, "queue init") err = mtastsdb.Init(false) tcheck(t, err, "mtastsdb init") err = tlsrptdb.Init() tcheck(t, err, "tlsrptdb init") + switchStop := store.Switchboard() + acc, err := store.OpenAccount(log, "mjl", false) tcheck(t, err, "open account") err = acc.SetPassword(log, "testtest") tcheck(t, err, "set password") - switchStop := store.Switchboard() - mox.Shutdown, mox.ShutdownCancel = context.WithCancel(ctxbg) + return acc, func() { acc.Close() acc.WaitClosed() diff --git a/smtpserver/alias_test.go b/smtpserver/alias_test.go index 5efcbcc..5c53398 100644 --- a/smtpserver/alias_test.go +++ b/smtpserver/alias_test.go @@ -81,7 +81,7 @@ func TestAliasSubmitMsgFromDenied(t *testing.T) { tcheck(t, err, "set password") err = acc.Close() tcheck(t, err, "close account") - acc.CheckClosed() + acc.WaitClosed() ts.submission = true ts.user = "☺@mox.example" diff --git a/smtpserver/fuzz_test.go b/smtpserver/fuzz_test.go index f03f710..b532685 100644 --- a/smtpserver/fuzz_test.go +++ b/smtpserver/fuzz_test.go @@ -42,6 +42,7 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("store init: %v", err) } + defer store.Switchboard()() acc, err := store.OpenAccount(log, "mjl", false) if err != nil { @@ -55,7 +56,7 @@ func FuzzServer(f *testing.F) { if err != nil { f.Fatalf("set password: %v", err) } - 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 2878137..89393d0 100644 --- a/smtpserver/server_test.go +++ b/smtpserver/server_test.go @@ -155,15 +155,15 @@ func newTestServer(t *testing.T, configPath string, resolver dns.Resolver) *test err = store.Init(ctxbg) tcheck(t, err, "store init") + ts.switchStop = store.Switchboard() + err = queue.Init() + tcheck(t, err, "queue init") + ts.acc, err = store.OpenAccount(log, "mjl", false) tcheck(t, err, "open account") err = ts.acc.SetPassword(log, password0) tcheck(t, err, "set password") - ts.switchStop = store.Switchboard() - err = queue.Init() - tcheck(t, err, "queue init") - ts.comm = store.RegisterComm(ts.acc) return &ts @@ -177,15 +177,15 @@ func (ts *testserver) close() { tcheck(ts.t, err, "dmarcdb close") err = tlsrptdb.Close() tcheck(ts.t, err, "tlsrptdb close") - err = store.Close() - tcheck(ts.t, err, "store close") ts.comm.Unregister() queue.Shutdown() - ts.switchStop() err = ts.acc.Close() tcheck(ts.t, err, "closing account") ts.acc.WaitClosed() ts.acc = nil + ts.switchStop() + err = store.Close() + tcheck(ts.t, err, "store close") } func (ts *testserver) checkCount(mailboxName string, expect int) { @@ -1505,7 +1505,7 @@ func TestCatchall(t *testing.T) { tcheck(t, err, "open account") defer func() { acc.Close() - acc.CheckClosed() + acc.WaitClosed() }() n, err = bstore.QueryDB[store.Message](ctxbg, acc.DB).Count() tcheck(t, err, "checking delivered messages to catchall account") diff --git a/store/account.go b/store/account.go index dcec9e3..3a18483 100644 --- a/store/account.go +++ b/store/account.go @@ -965,8 +965,8 @@ var InitialUIDValidity = func() uint32 { } var openAccounts = struct { - names map[string]*Account sync.Mutex + names map[string]*Account }{ names: map[string]*Account{}, } @@ -1029,6 +1029,7 @@ func OpenAccount(log mlog.Log, name string, checkLoginDisabled bool) (*Account, } // openAccount opens an existing account, or creates it if it is missing. +// Called with openAccounts lock held. func openAccount(log mlog.Log, name string) (a *Account, rerr error) { dir := filepath.Join(mox.DataDirPath("accounts"), name) return OpenAccountDB(log, dir, name) @@ -1582,15 +1583,6 @@ func (a *Account) WaitClosed() { <-a.closed } -// CheckClosed asserts that the account has a zero reference count. For use in tests. -func (a *Account) CheckClosed() { - openAccounts.Lock() - defer openAccounts.Unlock() - if a.nused != 0 { - panic(fmt.Sprintf("account still in use, %d refs", a.nused)) - } -} - // Close reduces the reference count, and closes the database connection when // it was the last user. func (a *Account) Close() error { diff --git a/store/account_test.go b/store/account_test.go index 5a32261..4c9e00f 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -41,6 +41,13 @@ func TestMailbox(t *testing.T) { os.RemoveAll("../testdata/store/data") mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf") mox.MustLoadConfig(true, false) + err := Init(ctxbg) + tcheck(t, err, "init") + defer func() { + err := Close() + tcheck(t, err, "close") + }() + defer Switchboard()() acc, err := OpenAccount(log, "mjl", false) tcheck(t, err, "open account") defer func() { @@ -48,7 +55,6 @@ func TestMailbox(t *testing.T) { tcheck(t, err, "closing account") acc.WaitClosed() }() - defer Switchboard()() msgFile, err := CreateMessageTemp(log, "account-test") tcheck(t, err, "create temp message file") @@ -316,12 +322,20 @@ func TestNextMessageID(t *testing.T) { os.RemoveAll("../testdata/store/data") mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf") mox.MustLoadConfig(true, false) + err := Init(ctxbg) + tcheck(t, err, "init") + defer func() { + err := Close() + tcheck(t, err, "close") + }() + defer Switchboard()() // Ensure account exists. acc, err := OpenAccount(log, "mjl", false) tcheck(t, err, "open account") err = acc.Close() tcheck(t, err, "closing account") + acc.WaitClosed() acc = nil // Create file on disk to occupy the first Message.ID that would otherwise be used for deliveries.. @@ -342,7 +356,6 @@ func TestNextMessageID(t *testing.T) { // Open account. This should increase the next message ID. acc, err = OpenAccount(log, "mjl", false) tcheck(t, err, "open account") - defer Switchboard()() // Deliver a message. It should get ID 2. mf, err := CreateMessageTemp(log, "account-test") @@ -369,6 +382,7 @@ func TestNextMessageID(t *testing.T) { err = acc.Close() tcheck(t, err, "closing account") + acc.WaitClosed() // Try again, but also create next message directory, but no file. os.MkdirAll(filepath.Join(msgDir, "b"), 0700) diff --git a/store/export_test.go b/store/export_test.go index 06afbca..951092b 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -24,14 +24,14 @@ func TestExport(t *testing.T) { os.RemoveAll("../testdata/store/data") mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf") mox.MustLoadConfig(true, false) + defer Switchboard()() acc, err := OpenAccount(pkglog, "mjl", false) tcheck(t, err, "open account") defer func() { err := acc.Close() log.Check(err, "closing account") - acc.CheckClosed() + acc.WaitClosed() }() - defer Switchboard()() msgFile, err := CreateMessageTemp(pkglog, "mox-test-export") tcheck(t, err, "create temp") diff --git a/store/threads_test.go b/store/threads_test.go index ac1c8d9..1715c7c 100644 --- a/store/threads_test.go +++ b/store/threads_test.go @@ -19,14 +19,15 @@ func TestThreadingUpgrade(t *testing.T) { os.RemoveAll("../testdata/store/data") mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf") mox.MustLoadConfig(true, false) + defer Switchboard()() + acc, err := OpenAccount(log, "mjl", true) tcheck(t, err, "open account") defer func() { err = acc.Close() tcheck(t, err, "closing account") - acc.CheckClosed() + acc.WaitClosed() }() - defer Switchboard()() // New account already has threading. Add some messages, check the threading. deliver := func(recv time.Time, s string, expThreadID int64) Message { @@ -120,7 +121,7 @@ func TestThreadingUpgrade(t *testing.T) { dbpath := acc.DBPath err = acc.Close() tcheck(t, err, "close account") - acc.CheckClosed() + acc.WaitClosed() // Now clear the threading upgrade, and the threading fields and close the account. // We open the database file directly, so we don't trigger the consistency checker. diff --git a/webaccount/account_test.go b/webaccount/account_test.go index 9283b58..5b13e2c 100644 --- a/webaccount/account_test.go +++ b/webaccount/account_test.go @@ -93,6 +93,7 @@ func tcompare(t *testing.T, got, expect any) { } func TestAccount(t *testing.T) { + log := mlog.New("webaccount", nil) os.RemoveAll("../testdata/httpaccount/data") mox.ConfigStaticPath = filepath.FromSlash("../testdata/httpaccount/mox.conf") mox.ConfigDynamicPath = filepath.Join(filepath.Dir(mox.ConfigStaticPath), "domains.conf") @@ -103,7 +104,8 @@ func TestAccount(t *testing.T) { err := store.Close() tcheck(t, err, "store close") }() - log := mlog.New("webaccount", nil) + defer store.Switchboard()() + acc, err := store.OpenAccount(log, "mjl☺", false) tcheck(t, err, "open account") err = acc.SetPassword(log, "test1234") @@ -113,7 +115,6 @@ func TestAccount(t *testing.T) { tcheck(t, err, "closing account") acc.WaitClosed() }() - defer store.Switchboard()() api := Account{cookiePath: "/account/"} apiHandler, err := makeSherpaHandler(api.cookiePath, false) diff --git a/webmail/api_test.go b/webmail/api_test.go index 171af43..63732ec 100644 --- a/webmail/api_test.go +++ b/webmail/api_test.go @@ -56,13 +56,13 @@ func TestAPI(t *testing.T) { mox.ConfigStaticPath = filepath.FromSlash("../testdata/webmail/mox.conf") mox.ConfigDynamicPath = filepath.FromSlash("../testdata/webmail/domains.conf") mox.MustLoadConfig(true, false) - defer store.Switchboard()() err := store.Init(ctxbg) tcheck(t, err, "store init") defer func() { err := store.Close() tcheck(t, err, "store close") }() + defer store.Switchboard()() log := mlog.New("webmail", nil) err = mtastsdb.Init(false) diff --git a/webmail/view_test.go b/webmail/view_test.go index bad209b..7f68deb 100644 --- a/webmail/view_test.go +++ b/webmail/view_test.go @@ -29,13 +29,13 @@ func TestView(t *testing.T) { mox.Context = ctxbg mox.ConfigStaticPath = filepath.FromSlash("../testdata/webmail/mox.conf") mox.MustLoadConfig(true, false) - defer store.Switchboard()() err := store.Init(ctxbg) tcheck(t, err, "store init") defer func() { err := store.Close() tcheck(t, err, "store close") }() + defer store.Switchboard()() log := mlog.New("webmail", nil) acc, err := store.OpenAccount(log, "mjl", false) diff --git a/webmail/webmail_test.go b/webmail/webmail_test.go index d90ca3e..56485b9 100644 --- a/webmail/webmail_test.go +++ b/webmail/webmail_test.go @@ -308,13 +308,13 @@ func TestWebmail(t *testing.T) { mox.Context = ctxbg mox.ConfigStaticPath = filepath.FromSlash("../testdata/webmail/mox.conf") mox.MustLoadConfig(true, false) - defer store.Switchboard()() err := store.Init(ctxbg) tcheck(t, err, "store init") defer func() { err := store.Close() tcheck(t, err, "store close") }() + defer store.Switchboard()() log := mlog.New("webmail", nil) acc, err := store.OpenAccount(pkglog, "mjl", false) @@ -324,7 +324,7 @@ func TestWebmail(t *testing.T) { defer func() { err := acc.Close() pkglog.Check(err, "closing account") - acc.CheckClosed() + acc.WaitClosed() }() api := Webmail{maxMessageSize: 1024 * 1024, cookiePath: "/webmail/"}