From a89b6bbca2cd5fa68c19373f1ffe3566e6e5000a Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Wed, 28 Feb 2018 14:21:39 -0800 Subject: [PATCH 1/3] Fix change log tag format --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df7e53f..d3a22a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,7 +98,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). specific message. [Unreleased]: https://github.com/jhillyerd/inbucket/compare/master...develop -[v1.3.0]: https://github.com/jhillyerd/inbucket/compare/1.2.0...1.3.0 +[v1.3.0]: https://github.com/jhillyerd/inbucket/compare/v1.2.0...v1.3.0 [v1.2.0]: https://github.com/jhillyerd/inbucket/compare/1.2.0-rc2...1.2.0 [v1.2.0-rc2]: https://github.com/jhillyerd/inbucket/compare/1.2.0-rc1...1.2.0-rc2 [v1.2.0-rc1]: https://github.com/jhillyerd/inbucket/compare/1.1.0...1.2.0-rc1 From a3877e4f4b49efd8026e504175d31d92927523f2 Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Fri, 9 Mar 2018 14:02:15 -0800 Subject: [PATCH 2/3] datastore: Concurrency fix, closes #77 --- datastore/datastore.go | 3 +++ datastore/lock.go | 19 +++++++++++++ datastore/lock_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++ datastore/testing.go | 5 ++++ filestore/fstore.go | 10 +++++++ smtpd/handler.go | 14 +++++++++- 6 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 datastore/lock.go create mode 100644 datastore/lock_test.go diff --git a/datastore/datastore.go b/datastore/datastore.go index 67aebf6..a9bcb57 100644 --- a/datastore/datastore.go +++ b/datastore/datastore.go @@ -5,6 +5,7 @@ import ( "errors" "io" "net/mail" + "sync" "time" "github.com/jhillyerd/enmime" @@ -22,6 +23,8 @@ var ( type DataStore interface { MailboxFor(emailAddress string) (Mailbox, error) AllMailboxes() ([]Mailbox, error) + // LockFor is a temporary hack to fix #77 until Datastore revamp + LockFor(emailAddress string) (*sync.RWMutex, error) } // Mailbox is an interface to get and manipulate messages in a DataStore diff --git a/datastore/lock.go b/datastore/lock.go new file mode 100644 index 0000000..a08dacc --- /dev/null +++ b/datastore/lock.go @@ -0,0 +1,19 @@ +package datastore + +import ( + "strconv" + "sync" +) + +type HashLock [4096]sync.RWMutex + +func (h *HashLock) Get(hash string) *sync.RWMutex { + if len(hash) < 3 { + return nil + } + i, err := strconv.ParseInt(hash[0:3], 16, 0) + if err != nil { + return nil + } + return &h[i] +} diff --git a/datastore/lock_test.go b/datastore/lock_test.go new file mode 100644 index 0000000..2da536a --- /dev/null +++ b/datastore/lock_test.go @@ -0,0 +1,61 @@ +package datastore_test + +import ( + "testing" + + "github.com/jhillyerd/inbucket/datastore" +) + +func TestHashLock(t *testing.T) { + hl := &datastore.HashLock{} + + // Invalid hashes + testCases := []struct { + name, input string + }{ + {"empty", ""}, + {"short", "a0"}, + {"badhex", "zzzzzzzzzzzzzzzzzzzzzzz"}, + } + for _, tc := range testCases { + t.Run(tc.input, func(t *testing.T) { + l := hl.Get(tc.input) + if l != nil { + t.Errorf("Expected nil lock for %s %q, got %v", tc.name, tc.input, l) + } + }) + } + + // Valid hashes + testStrings := []string{ + "deadbeef", + "00000000", + "ffffffff", + } + for _, ts := range testStrings { + t.Run(ts, func(t *testing.T) { + l := hl.Get(ts) + if l == nil { + t.Errorf("Expeced non-nil lock for hex string %q", ts) + } + }) + } + + a := hl.Get("deadbeef") + b := hl.Get("deadbeef") + if a != b { + t.Errorf("Expected identical locks for identical hashes, got: %p != %p", a, b) + } + + a = hl.Get("deadbeef") + b = hl.Get("d3adb33f") + if a == b { + t.Errorf("Expected different locks for different hashes, got: %p == %p", a, b) + } + + a = hl.Get("deadbeef") + b = hl.Get("deadb33f") + if a != b { + t.Errorf("Expected identical locks for identical leading hashes, got: %p != %p", a, b) + } +} diff --git a/datastore/testing.go b/datastore/testing.go index 23474f6..8e0799c 100644 --- a/datastore/testing.go +++ b/datastore/testing.go @@ -3,6 +3,7 @@ package datastore import ( "io" "net/mail" + "sync" "time" "github.com/jhillyerd/enmime" @@ -26,6 +27,10 @@ func (m *MockDataStore) AllMailboxes() ([]Mailbox, error) { return args.Get(0).([]Mailbox), args.Error(1) } +func (m *MockDataStore) LockFor(name string) (*sync.RWMutex, error) { + return &sync.RWMutex{}, nil +} + // MockMailbox is a shared mock for unit testing type MockMailbox struct { mock.Mock diff --git a/filestore/fstore.go b/filestore/fstore.go index 4ac7bc1..7b15f27 100644 --- a/filestore/fstore.go +++ b/filestore/fstore.go @@ -51,6 +51,7 @@ func countGenerator(c chan int) { // FileDataStore implements DataStore aand is the root of the mail storage // hiearchy. It provides access to Mailbox objects type FileDataStore struct { + hashLock datastore.HashLock path string mailPath string messageCap int @@ -139,6 +140,15 @@ func (ds *FileDataStore) AllMailboxes() ([]datastore.Mailbox, error) { return mailboxes, nil } +func (ds *FileDataStore) LockFor(emailAddress string) (*sync.RWMutex, error) { + name, err := stringutil.ParseMailboxName(emailAddress) + if err != nil { + return nil, err + } + hash := stringutil.HashMailboxName(name) + return ds.hashLock.Get(hash), nil +} + // FileMailbox implements Mailbox, manages the mail for a specific user and // correlates to a particular directory on disk. type FileMailbox struct { diff --git a/smtpd/handler.go b/smtpd/handler.go index f6c4397..cbae977 100644 --- a/smtpd/handler.go +++ b/smtpd/handler.go @@ -402,7 +402,19 @@ func (ss *Session) dataHandler() { if ss.server.storeMessages { // Create a message for each valid recipient for _, r := range recipients { - if ok := ss.deliverMessage(r, msgBuf); ok { + // TODO temporary hack to fix #77 until datastore revamp + mu, err := ss.server.dataStore.LockFor(r.localPart) + if err != nil { + ss.logError("Failed to get lock for %q: %s", r.localPart, err) + // Delivery failure + ss.send(fmt.Sprintf("451 Failed to store message for %v", r.localPart)) + ss.reset() + return + } + mu.Lock() + ok := ss.deliverMessage(r, msgBuf) + mu.Unlock() + if ok { expReceivedTotal.Add(1) } else { // Delivery failure From 019e66d7988c1ab1ceb8b2ac85a0d24d5f540a9d Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Sat, 10 Mar 2018 10:06:09 -0800 Subject: [PATCH 3/3] Update change log for 1.3.1 --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3a22a5..7874198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ Change Log All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [v1.3.1] - 2018-03-10 + +### Fixed + +- Adding additional locking during message delivery to prevent race condition + that could lose messages. + + ## [v1.3.0] - 2018-02-28 ### Added @@ -13,6 +21,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Reverse message display sort order in the UI; now newest first. + ## [v1.2.0] - 2017-12-27 ### Changed @@ -31,6 +40,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). types - Fixed panic when `monitor.history` set to 0 + ## [v1.2.0-rc1] - 2017-01-29 ### Added @@ -57,6 +67,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Allow increased local-part length of 128 chars for Mailgun - RedHat and Ubuntu now use systemd instead of legacy init systems + ## [v1.1.0] - 2016-09-03 ### Added @@ -65,6 +76,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Log and continue when unable to delete oldest message during cap enforcement + ## [v1.1.0-rc2] - 2016-03-06 ### Added @@ -75,6 +87,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Shutdown hang in retention scanner - Display empty subject as `(No Subject)` + ## [v1.1.0-rc1] - 2016-03-04 ### Added @@ -89,6 +102,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - RESTful API moved to `/api/v1` base URI - More graceful shutdown on Ctrl-C or when errors encountered + ## [v1.0] - 2014-04-14 ### Added @@ -98,6 +112,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). specific message. [Unreleased]: https://github.com/jhillyerd/inbucket/compare/master...develop +[v1.3.1]: https://github.com/jhillyerd/inbucket/compare/v1.3.0...v1.3.1 [v1.3.0]: https://github.com/jhillyerd/inbucket/compare/v1.2.0...v1.3.0 [v1.2.0]: https://github.com/jhillyerd/inbucket/compare/1.2.0-rc2...1.2.0 [v1.2.0-rc2]: https://github.com/jhillyerd/inbucket/compare/1.2.0-rc1...1.2.0-rc2