From 2c1283f032cb3a744e7e2d8a855b49ec21e73a34 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Fri, 11 Apr 2025 21:04:13 +0200 Subject: [PATCH] imapclient: clean up function signature of New, allowing for future options too --- ctl_test.go | 7 +++++- imapclient/client.go | 50 +++++++++++++++++++++++-------------- imapserver/compress_test.go | 5 ---- imapserver/fuzz_test.go | 7 +++++- imapserver/server_test.go | 8 ++++-- integration_test.go | 5 +++- 6 files changed, 53 insertions(+), 29 deletions(-) diff --git a/ctl_test.go b/ctl_test.go index f2a3feb..971ad51 100644 --- a/ctl_test.go +++ b/ctl_test.go @@ -9,6 +9,7 @@ import ( "crypto/x509" "flag" "fmt" + "log/slog" "math/big" "net" "os" @@ -524,7 +525,11 @@ func TestCtl(t *testing.T) { testctl(func(xctl *ctl) { a, b := net.Pipe() go func() { - client, err := imapclient.New(mox.Cid(), a, true) + opts := imapclient.Opts{ + Logger: slog.Default().With("cid", mox.Cid()), + Error: func(err error) { panic(err) }, + } + client, err := imapclient.New(a, &opts) tcheck(t, err, "new imapclient") client.Select("inbox") client.Logout() diff --git a/imapclient/client.go b/imapclient/client.go index 969db05..e6a9bf9 100644 --- a/imapclient/client.go +++ b/imapclient/client.go @@ -45,7 +45,7 @@ type Conn struct { xtw *moxio.TraceWriter log mlog.Log - panic bool + errHandle func(err error) // If set, called for all errors. Can panic. Used for imapserver tests. tagGen int record bool // If true, bytes read are added to recordBuf. recorded() resets. recordBuf []byte @@ -67,28 +67,46 @@ func (e Error) Unwrap() error { return e.err } +// Opts has optional fields that influence behaviour of a Conn. +type Opts struct { + Logger *slog.Logger + + // Error is called for both IMAP-level and connection-level errors. Is allowed to + // call panic. + Error func(err error) +} + // New creates a new client on conn. // -// If xpanic is true, functions that would return an error instead panic. For parse -// errors, the resulting stack traces show typically show what was being parsed. -// // The initial untagged greeting response is read and must be "OK" or // "PREAUTH". If preauth, the connection is already in authenticated state, // typically through TLS client certificate. This is indicated in Conn.Preauth. -func New(cid int64, conn net.Conn, xpanic bool) (client *Conn, rerr error) { - log := mlog.New("imapclient", nil).WithCid(cid) +// +// Logging is written to log, in particular IMAP protocol traces are written with +// prefixes "CR: " and "CW: " (client read/write) as quoted strings at levels +// Debug-4, with authentication messages at Debug-6 and (user) data at level +// Debug-8. +func New(conn net.Conn, opts *Opts) (client *Conn, rerr error) { c := Conn{ conn: conn, - log: log, - panic: xpanic, CapAvailable: map[Capability]struct{}{}, CapEnabled: map[Capability]struct{}{}, } - c.tr = moxio.NewTraceReader(log, "CR: ", &c) + + var clog *slog.Logger + if opts != nil { + c.errHandle = opts.Error + clog = opts.Logger + } else { + clog = slog.Default() + } + c.log = mlog.New("imapclient", clog) + + c.tr = moxio.NewTraceReader(c.log, "CR: ", &c) c.br = bufio.NewReader(c.tr) // Writes are buffered and write to Conn, which may panic. - c.xtw = moxio.NewTraceWriter(log, "CW: ", &c) + c.xtw = moxio.NewTraceWriter(c.log, "CW: ", &c) c.xbw = bufio.NewWriter(c.xtw) defer c.recover(&rerr) @@ -116,10 +134,6 @@ func New(cid int64, conn net.Conn, xpanic bool) (client *Conn, rerr error) { } func (c *Conn) recover(rerr *error) { - if c.panic { - return - } - x := recover() if x == nil { return @@ -128,6 +142,9 @@ func (c *Conn) recover(rerr *error) { if !ok { panic(x) } + if c.errHandle != nil { + c.errHandle(err) + } *rerr = err } @@ -201,11 +218,6 @@ func (c *Conn) xtracewrite(level slog.Level) func() { } } -// SetPanic sets whether errors cause a panic instead of returning errors. -func (c *Conn) SetPanic(panic bool) { - c.panic = panic -} - // Close closes the connection, flushing and closing any compression and TLS layer. // // You may want to call Logout first. Closing a connection with a mailbox with diff --git a/imapserver/compress_test.go b/imapserver/compress_test.go index 3d0d7db..1883f75 100644 --- a/imapserver/compress_test.go +++ b/imapserver/compress_test.go @@ -40,11 +40,6 @@ func TestCompressStartTLS(t *testing.T) { tc.transactf("ok", "append inbox (\\seen) {%d+}\r\n%s", len(exampleMsg), exampleMsg) tc.transactf("ok", "noop") tc.transactf("ok", "fetch 1 body.peek[1]") - - // Prevent client.Close from failing the test. The client first closes the - // compression stream, which causes the server to close the connection, after which - // the messages to close the TLS connection are written to a closed socket. - tc.client.SetPanic(false) } func TestCompressBreak(t *testing.T) { diff --git a/imapserver/fuzz_test.go b/imapserver/fuzz_test.go index 20f25b1..b31c8dc 100644 --- a/imapserver/fuzz_test.go +++ b/imapserver/fuzz_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "log/slog" "net" "os" "path/filepath" @@ -126,7 +127,11 @@ func FuzzServer(f *testing.F) { err := clientConn.SetDeadline(time.Now().Add(time.Second)) flog(err, "set client deadline") - client, _ := imapclient.New(mox.Cid(), clientConn, true) + opts := imapclient.Opts{ + Logger: slog.Default().With("cid", mox.Cid()), + Error: func(err error) { panic(err) }, + } + client, _ := imapclient.New(clientConn, &opts) for _, cmd := range cmds { client.Commandf("", "%s", cmd) diff --git a/imapserver/server_test.go b/imapserver/server_test.go index 442f5aa..4963a88 100644 --- a/imapserver/server_test.go +++ b/imapserver/server_test.go @@ -7,6 +7,7 @@ import ( "crypto/tls" "crypto/x509" "fmt" + "log/slog" "math/big" "net" "os" @@ -536,8 +537,11 @@ func startArgsMore(t *testing.T, uidonly, first, immediateTLS bool, serverConfig serve("test", cid, serverConfig, serverConn, immediateTLS, allowLoginWithoutTLS, viaHTTPS, "") close(done) }() - client, err := imapclient.New(connCounter, clientConn, true) - tcheck(t, err, "new client") + opts := imapclient.Opts{ + Logger: slog.Default().With("cid", connCounter), + Error: func(err error) { panic(err) }, + } + client, _ := imapclient.New(clientConn, &opts) tc := &testconn{t: t, conn: clientConn, client: client, uidonly: uidonly, done: done, serverConn: serverConn, account: acc} if first { tc.switchStop = switchStop diff --git a/integration_test.go b/integration_test.go index 34f642f..18abca2 100644 --- a/integration_test.go +++ b/integration_test.go @@ -64,7 +64,10 @@ func TestDeliver(t *testing.T) { tcheck(t, err, "dial imap") defer imapconn.Close() - imapc, err := imapclient.New(mox.Cid(), imapconn, false) + opts := imapclient.Opts{ + Logger: slog.Default().With("cid", mox.Cid()), + } + imapc, err := imapclient.New(imapconn, &opts) tcheck(t, err, "new imapclient") _, _, err = imapc.Login(imapuser, imappassword)