Feedback-ID: i3ad947a1:Fastmail
From: Tim Culverhouse <tim@timculverhouse.com>
To: ~rockorager/offmap@lists.sr.ht
Cc: Tim Culverhouse <tim@timculverhouse.com>,
	inwit <inwit@sindominio.net>
Subject: [PATCH offmap v2] email: don't keep files open after downloading
Date: Wed, 30 Nov 2022 10:38:25 -0600
Message-Id: <20221130163824.102652-1-tim@timculverhouse.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Store the absolute filename to the downloaded email instead of keeping
an open file descriptor.

Use os.Rename when creating maildir emails to reduce I/O.

Fix tests.

Reported-by: inwit <inwit@sindominio.net>
Signed-off-by: Tim Culverhouse <tim@timculverhouse.com>
---
v2: read the entire file into a buffer for reading

 email.go               | 28 +++++++++++++++++++---------
 imap/imap.go           |  8 +-------
 maildir/maildir.go     |  9 ++-------
 maildir/update.go      | 10 ++++++----
 maildir/update_test.go | 15 ++++++++++-----
 5 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/email.go b/email.go
index c11fd82bca8c..7d9fdf50627d 100644
--- a/email.go
+++ b/email.go
@@ -1,8 +1,10 @@
 package offmap
 
 import (
+	"bytes"
 	"fmt"
 	"io"
+	"os"
 	"time"
 
 	"github.com/emersion/go-maildir"
@@ -23,8 +25,9 @@ type Email struct {
 	// Date is the best known time for the email creation
 	Date time.Time `json:"-"`
 
-	// Body is the RFC822 body of the email (ie, the entire email)
-	body io.ReadCloser
+	// filename is the absolute path of the email on disk
+	filename string
+	body     io.Reader
 	// size is the size of email in bytes
 	size int
 }
@@ -40,17 +43,28 @@ func NewEmail(mbox *Mailbox, uid uint32, key string, flags []maildir.Flag) *Emai
 	}
 }
 
-func (e *Email) SetBody(body io.ReadCloser) {
-	e.body = body
+func (e *Email) SetFilename(path string) {
+	e.filename = path
 }
 
 func (e *Email) Read(p []byte) (int, error) {
-	if e.body == nil {
+	if e.filename == "" {
 		return 0, fmt.Errorf("email: no body")
 	}
+	if e.body == nil {
+		b, err := os.ReadFile(e.filename)
+		if err != nil {
+			return 0, fmt.Errorf("email: %v", err)
+		}
+		e.body = bytes.NewBuffer(b)
+	}
 	return e.body.Read(p)
 }
 
+func (e *Email) Filename() string {
+	return e.filename
+}
+
 func (e *Email) SetSize(i int) {
 	e.size = i
 }
@@ -58,7 +72,3 @@ func (e *Email) SetSize(i int) {
 func (e *Email) Len() int {
 	return e.size
 }
-
-func (e *Email) Close() error {
-	return e.body.Close()
-}
diff --git a/imap/imap.go b/imap/imap.go
index 74e3eb55e032..4bf9e918a517 100644
--- a/imap/imap.go
+++ b/imap/imap.go
@@ -220,12 +220,6 @@ func (s *Store) DownloadEmail(emls []*offmap.Email) []*offmap.Email {
 					atomic.AddInt32(&dled, 1)
 					cur := atomic.LoadInt32(&dled)
 					f.Close()
-					// Reopen but with read only mode
-					f, err = os.Open(filepath)
-					if err != nil {
-						log.Errorf("imap: %v", err)
-						continue
-					}
 					emlMu.Lock()
 					eml, ok := emlMap[filename]
 					emlMu.Unlock()
@@ -233,7 +227,7 @@ func (s *Store) DownloadEmail(emls []*offmap.Email) []*offmap.Email {
 						log.Errorf("imap: %s: eml not found: %s", status.Name, filename)
 						continue
 					}
-					eml.SetBody(f)
+					eml.SetFilename(filepath)
 					log.Tracef("imap: (client %d) downloaded %d of %d", client.id, cur, total)
 				}
 				msgWg.Done()
diff --git a/maildir/maildir.go b/maildir/maildir.go
index 2682066e3fa4..5209332d285d 100644
--- a/maildir/maildir.go
+++ b/maildir/maildir.go
@@ -66,17 +66,12 @@ func (s *Store) DownloadEmail(emls []*offmap.Email) []*offmap.Email {
 			log.Errorf("maildir: couldn't find email %d", eml.UID)
 			continue
 		}
-		f, err := os.Open(filename)
+		fi, err := os.Stat(filename)
 		if err != nil {
 			log.Errorf("maildir: couldn't open file %d", eml.UID)
 			continue
 		}
-		fi, err := f.Stat()
-		if err != nil {
-			log.Errorf("maildir: couldn't open file %d", eml.UID)
-			continue
-		}
-		eml.SetBody(f)
+		eml.SetFilename(filename)
 		eml.Date = fi.ModTime()
 		eml.SetSize(int(fi.Size()))
 	}
diff --git a/maildir/update.go b/maildir/update.go
index 2e8a1c56a59c..dc0d6ac6e53b 100644
--- a/maildir/update.go
+++ b/maildir/update.go
@@ -2,7 +2,6 @@ package maildir
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"path"
 
@@ -109,13 +108,16 @@ func (s *Store) renameMaildir(orig string, dest string) error {
 func (s *Store) createEmail(mbox string, emls []*offmap.Email) error {
 	dir := maildir.Dir(path.Join(s.root, mbox))
 	for _, eml := range emls {
-		defer eml.Close()
 		key, wc, err := dir.Create(eml.Flags)
 		if err != nil {
 			return fmt.Errorf("maildir: could not create email: %v", err)
 		}
-		defer wc.Close()
-		_, err = io.Copy(wc, eml)
+		wc.Close()
+		dest, err := dir.Filename(key)
+		if err != nil {
+			return fmt.Errorf("maildir: could not write email: %v", err)
+		}
+		err = os.Rename(eml.Filename(), dest)
 		if err != nil {
 			return fmt.Errorf("maildir: could not write email: %v", err)
 		}
diff --git a/maildir/update_test.go b/maildir/update_test.go
index fbbe2f23c742..6faa060d47fb 100644
--- a/maildir/update_test.go
+++ b/maildir/update_test.go
@@ -1,8 +1,8 @@
 package maildir
 
 import (
-	"bytes"
-	"io"
+	"os"
+	"path"
 	"testing"
 
 	"git.sr.ht/~rockorager/offmap"
@@ -75,9 +75,14 @@ func TestCreateEmail(t *testing.T) {
 	mailbox := mboxes["Inbox"]
 	eml := offmap.NewEmail(mailbox, 0, "key", []maildir.Flag{'S'})
 
-	buf := bytes.NewBufferString("Email body")
-	rc := io.NopCloser(buf)
-	eml.SetBody(rc)
+	tmpEml := path.Join(t.TempDir(), "tmpEmail")
+	f, err := os.Create(tmpEml)
+	if err != nil {
+		t.Fatal(err)
+	}
+	f.WriteString("email body")
+	f.Close()
+	eml.SetFilename(tmpEml)
 
 	mboxes, err = store.readCurrentState()
 	assert.NoError(t, err)
-- 
2.38.1