More index.gob work

Use a global lock for reading/writing any index file. Possible bottleneck, but good enough for now. Don't create a mailbox directory until a message is being written to it.
2025-12-17 17:47:03 +00:00 · 2013-10-11 20:06:33 -07:00
parent 789105b9fd
commit 08f16db7ac
2 changed files with 198 additions and 97 deletions
--- a/smtpd/filestore.go
+++ b/smtpd/filestore.go
@@ -13,11 +13,17 @@ import (
 	"net/mail"
 	"os"
 	"path/filepath"
+	"sync"
 	"time"
 )

+// Name of index file in each mailbox
 const INDEX_FILE = "index.gob"

+// We lock this when reading/writing an index file, this is a bottleneck because
+// it's a single lock even if we have a million index files
+var indexLock = new(sync.RWMutex)
+
 var ErrNotWritable = errors.New("Message not writable")

 // Global because we only want one regardless of the number of DataStore objects
@@ -55,6 +61,10 @@ func NewFileDataStore() DataStore {
 		return nil
 	}
 	mailPath := filepath.Join(path, "mail")
+	if _, err := os.Stat(mailPath); err != nil {
+		// Mail datastore does not yet exist
+		os.MkdirAll(mailPath, 0770)
+	}
 	return &FileDataStore{path: path, mailPath: mailPath}
 }

@@ -67,19 +77,6 @@ func (ds *FileDataStore) MailboxFor(emailAddress string) (Mailbox, error) {
 	s2 := dir[0:6]
 	path := filepath.Join(ds.mailPath, s1, s2, dir)
 	indexPath := filepath.Join(path, INDEX_FILE)
-	if err := os.MkdirAll(path, 0770); err != nil {
-		log.LogError("Failed to create directory %v, %v", path, err)
-		return nil, err
-	}
-	if _, err := os.Stat(indexPath); err != nil {
-		// index does not yet exist, create empty one
-		if file, err := os.Create(indexPath); err != nil {
-			log.LogError("Failed to create index %v, %v", indexPath, err)
-			return nil, err
-		} else {
-			file.Close()
-		}
-	}

 	return &FileMailbox{store: ds, name: name, dirName: dir, path: path,
 		indexPath: indexPath}, nil
@@ -180,6 +177,16 @@ func (mb *FileMailbox) GetMessage(id string) (Message, error) {
 func (mb *FileMailbox) readIndex() error {
 	// Clear message slice, open index
 	mb.messages = mb.messages[:0]
+	// Lock for reading
+	indexLock.RLock()
+	defer indexLock.RUnlock()
+	// Check if index exists
+	if _, err := os.Stat(mb.indexPath); err != nil {
+		// Does not exist, but that's not an error in our world
+		log.LogTrace("Index %v does not exist (yet)", mb.indexPath)
+		mb.indexLoaded = true
+		return nil
+	}
 	file, err := os.Open(mb.indexPath)
 	if err != nil {
 		return err
@@ -207,8 +214,26 @@ func (mb *FileMailbox) readIndex() error {
 	return nil
 }

+// createDir checks for the presence of the path for this mailbox, creates it if needed
+func (mb *FileMailbox) createDir() error {
+	if _, err := os.Stat(mb.path); err != nil {
+		if err := os.MkdirAll(mb.path, 0770); err != nil {
+			log.LogError("Failed to create directory %v, %v", mb.path, err)
+			return err
+		}
+	}
+	return nil
+}
+
 // writeIndex overwrites the index on disk with the current mailbox data
 func (mb *FileMailbox) writeIndex() error {
+	// Lock for writing
+	indexLock.Lock()
+	defer indexLock.Unlock()
+	// Ensure mailbox directory exists
+	if err := mb.createDir(); err != nil {
+		return err
+	}
 	// Open index for writing
 	file, err := os.Create(mb.indexPath)
 	if err != nil {
@@ -349,6 +374,10 @@ func (m *FileMessage) Append(data []byte) error {
 	}
 	// Open file for writing if we haven't yet
 	if m.writer == nil {
+		// Ensure mailbox directory exists
+		if err := m.mailbox.createDir(); err != nil {
+			return err
+		}
 		file, err := os.Create(m.rawPath())
 		if err != nil {
 			// Set writable false just in case something calls me a million times
@@ -413,6 +442,7 @@ func (m *FileMessage) Delete() error {
 			break
 		}
 	}
+	m.mailbox.writeIndex()

 	log.LogTrace("Deleting %v", m.rawPath())
 	return os.Remove(m.rawPath())
--- a/smtpd/filestore_test.go
+++ b/smtpd/filestore_test.go
@@ -1,6 +1,7 @@
 package smtpd

 import (
+	"fmt"
 	"github.com/stretchr/testify/assert"
 	"io/ioutil"
 	"os"
@@ -9,105 +10,175 @@ import (
 	"time"
 )

-var mailboxNames = []string{"abby", "bill", "christa", "donald", "evelyn"}
-
+// Test FileDataStore.AllMailboxes()
 func TestFSAllMailboxes(t *testing.T) {
-	fds := setupDataStore()
-	defer teardownDataStore(fds)
+	ds := setupDataStore()
+	defer teardownDataStore(ds)

-	mboxes, err := fds.AllMailboxes()
+	for _, name := range []string{"abby", "bill", "christa", "donald", "evelyn"} {
+		// Create day old message
+		date := time.Now().Add(-24 * time.Hour)
+		deliverMessage(ds, name, "Old Message", date)
+
+		// Create current message
+		date = time.Now()
+		deliverMessage(ds, name, "New Message", date)
+	}
+
+	mboxes, err := ds.AllMailboxes()
 	assert.Nil(t, err)
 	assert.Equal(t, len(mboxes), 5)
 }

-// setupDataStore will build the following structure in a temporary
-// directory:
-//
-// /tmp/inbucket?????????
-// └── mail
-//     ├── 53e
-//     │   └── 53e11e
-//     │       └── 53e11eb7b24cc39e33733a0ff06640f1b39425ea
-//     │           ├── 20121024T164239-0000.gob
-//     │           ├── 20121024T164239-0000.raw
-//     │           ├── 20121025T164239-0000.gob
-//     │           └── 20121025T164239-0000.raw
-//     ├── 60c
-//     │   └── 60c596
-//     │       └── 60c5963a56da1425f133d28166ca4fe70dcb25f5
-//     │           ├── 20121024T164239-0000.gob
-//     │           ├── 20121024T164239-0000.raw
-//     │           ├── 20121025T164239-0000.gob
-//     │           └── 20121025T164239-0000.raw
-//     ├── 88d
-//     │   └── 88db92
-//     │       └── 88db9292c772b38311e1778f6f6b18216443abf0
-//     │           ├── 20121024T164239-0000.gob
-//     │           ├── 20121024T164239-0000.raw
-//     │           ├── 20121025T164239-0000.gob
-//     │           └── 20121025T164239-0000.raw
-//     ├── c69
-//     │   └── c692d6
-//     │       └── c692d6a10598e0a801576fdd4ecf3c37e45bfbc4
-//     │           ├── 20121024T164239-0000.gob
-//     │           ├── 20121024T164239-0000.raw
-//     │           ├── 20121025T164239-0000.gob
-//     │           └── 20121025T164239-0000.raw
-//     └── e76
-//         └── e76cef
-//             └── e76ceff3c47adb10f62b1acd7109f88fbd5e9ca7
-//                 ├── 20121024T164239-0000.gob
-//                 ├── 20121024T164239-0000.raw
-//                 ├── 20121025T164239-0000.gob
-//                 └── 20121025T164239-0000.raw
-func setupDataStore() *FileDataStore {
-	// Build fake SMTP message for delivery
-	testMsg := make([]byte, 0, 300)
-	testMsg = append(testMsg, []byte("To: somebody@host\r\n")...)
-	testMsg = append(testMsg, []byte("From: somebodyelse@host\r\n")...)
-	testMsg = append(testMsg, []byte("Subject: test message\r\n")...)
-	testMsg = append(testMsg, []byte("\r\n")...)
-	testMsg = append(testMsg, []byte("Test Body\r\n")...)
+// Test delivering several messages to the same mailbox, meanwhile querying its
+// contents with a new mailbox object each time
+func TestFSDeliverMany(t *testing.T) {
+	ds := setupDataStore()
+	defer teardownDataStore(ds)

+	mbName := "fred"
+	subjects := []string{"alpha", "bravo", "charlie", "delta", "echo"}
+
+	for i, subj := range subjects {
+		// Check number of messages
+		mb, err := ds.MailboxFor(mbName)
+		if err != nil {
+			panic(err)
+		}
+		msgs, err := mb.GetMessages()
+		if err != nil {
+			panic(err)
+		}
+		assert.Equal(t, i, len(msgs), "Expected %v message(s), but got %v", i, len(msgs))
+
+		// Add a message
+		deliverMessage(ds, mbName, subj, time.Now())
+	}
+
+	mb, err := ds.MailboxFor(mbName)
+	if err != nil {
+		panic(err)
+	}
+	msgs, err := mb.GetMessages()
+	if err != nil {
+		panic(err)
+	}
+	assert.Equal(t, len(subjects), len(msgs), "Expected %v message(s), but got %v",
+		len(subjects), len(msgs))
+
+	// Confirm delivery order
+	for i, expect := range subjects {
+		subj := msgs[i].Subject()
+		assert.Equal(t, expect, subj, "Expected subject %q, got %q", expect, subj)
+	}
+}
+
+// Test deleting messages
+func TestFSDelete(t *testing.T) {
+	ds := setupDataStore()
+	defer teardownDataStore(ds)
+
+	mbName := "fred"
+	subjects := []string{"alpha", "bravo", "charlie", "delta", "echo"}
+
+	for _, subj := range subjects {
+		// Add a message
+		deliverMessage(ds, mbName, subj, time.Now())
+	}
+
+	mb, err := ds.MailboxFor(mbName)
+	if err != nil {
+		panic(err)
+	}
+	msgs, err := mb.GetMessages()
+	if err != nil {
+		panic(err)
+	}
+	assert.Equal(t, len(subjects), len(msgs), "Expected %v message(s), but got %v",
+		len(subjects), len(msgs))
+
+	// Delete a couple messages
+	msgs[1].Delete()
+	msgs[3].Delete()
+
+	// Confirm deletion
+	mb, err = ds.MailboxFor(mbName)
+	if err != nil {
+		panic(err)
+	}
+	msgs, err = mb.GetMessages()
+	if err != nil {
+		panic(err)
+	}
+
+	subjects = []string{"alpha", "charlie", "echo"}
+	assert.Equal(t, len(subjects), len(msgs), "Expected %v message(s), but got %v",
+		len(subjects), len(msgs))
+	for i, expect := range subjects {
+		subj := msgs[i].Subject()
+		assert.Equal(t, expect, subj, "Expected subject %q, got %q", expect, subj)
+	}
+
+	// Try appending one more
+	deliverMessage(ds, mbName, "foxtrot", time.Now())
+
+	mb, err = ds.MailboxFor(mbName)
+	if err != nil {
+		panic(err)
+	}
+	msgs, err = mb.GetMessages()
+	if err != nil {
+		panic(err)
+	}
+
+	subjects = []string{"alpha", "charlie", "echo", "foxtrot"}
+	assert.Equal(t, len(subjects), len(msgs), "Expected %v message(s), but got %v",
+		len(subjects), len(msgs))
+	for i, expect := range subjects {
+		subj := msgs[i].Subject()
+		assert.Equal(t, expect, subj, "Expected subject %q, got %q", expect, subj)
+	}
+
+}
+
+// setupDataStore creates a new FileDataStore in a temporary directory
+func setupDataStore() *FileDataStore {
 	path, err := ioutil.TempDir("", "inbucket")
 	if err != nil {
 		panic(err)
 	}
 	mailPath := filepath.Join(path, "mail")
-	ds := &FileDataStore{path: path, mailPath: mailPath}
+	return &FileDataStore{path: path, mailPath: mailPath}
+}

-	for _, name := range mailboxNames {
-		mb, err := ds.MailboxFor(name)
-		if err != nil {
-			panic(err)
-		}
-		// Create day old message
-		date := time.Now().Add(-24 * time.Hour)
-		msg := &FileMessage{
-			mailbox:  mb.(*FileMailbox),
-			writable: true,
-			Fdate:    date,
-			Fid:      generatePrefix(date) + "-0000",
-		}
-		msg.Append(testMsg)
-		if err = msg.Close(); err != nil {
-			panic(err)
-		}
+// deliverMessage creates and delivers a message to the specific mailbox, returning
+// the size of the generated message.
+func deliverMessage(ds *FileDataStore, mbName string, subject string, date time.Time) int {
+	// Build fake SMTP message for delivery
+	testMsg := make([]byte, 0, 300)
+	testMsg = append(testMsg, []byte("To: somebody@host\r\n")...)
+	testMsg = append(testMsg, []byte("From: somebodyelse@host\r\n")...)
+	testMsg = append(testMsg, []byte(fmt.Sprintf("Subject: %s\r\n", subject))...)
+	testMsg = append(testMsg, []byte("\r\n")...)
+	testMsg = append(testMsg, []byte("Test Body\r\n")...)

-		// Create current message
-		date = time.Now()
-		msg = &FileMessage{
-			mailbox:  mb.(*FileMailbox),
-			writable: true,
-			Fdate:    date,
-			Fid:      generatePrefix(date) + "-0000",
-		}
-		msg.Append(testMsg)
-		if err = msg.Close(); err != nil {
-			panic(err)
-		}
+	mb, err := ds.MailboxFor(mbName)
+	if err != nil {
+		panic(err)
 	}
-	return ds
+	// Create day old message
+	msg := &FileMessage{
+		mailbox:  mb.(*FileMailbox),
+		writable: true,
+		Fdate:    date,
+		Fid:      generateId(date),
+	}
+	msg.Append(testMsg)
+	if err = msg.Close(); err != nil {
+		panic(err)
+	}
+
+	return len(testMsg)
 }

 func teardownDataStore(ds *FileDataStore) {