From 08f16db7ac38155d76b4d30fb0cefd2f9853d5b3 Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Fri, 11 Oct 2013 20:06:33 -0700 Subject: [PATCH] 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. --- smtpd/filestore.go | 56 +++++++--- smtpd/filestore_test.go | 239 ++++++++++++++++++++++++++-------------- 2 files changed, 198 insertions(+), 97 deletions(-) diff --git a/smtpd/filestore.go b/smtpd/filestore.go index c98ace2..a515d84 100644 --- 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()) diff --git a/smtpd/filestore_test.go b/smtpd/filestore_test.go index 4c933b3..6ae9a83 100644 --- 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) {