GODT-1840: Safe map for mailboxID cache

internal/imap/map.go

@@ -0,0 +1,44 @@
+// Copyright (c) 2022 Proton AG
+//
+// This file is part of Proton Mail Bridge.
+//
+// Proton Mail Bridge is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Proton Mail Bridge is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Proton Mail Bridge. If not, see <https://www.gnu.org/licenses/>.
+
+package imap
+
+import "sync"
+
+type safeMapOfStrings struct {
+	data  map[string]string
+	mutex sync.RWMutex
+}
+
+func newSafeMapOfString() safeMapOfStrings {
+	return safeMapOfStrings{
+		data:  map[string]string{},
+		mutex: sync.RWMutex{},
+	}
+}
+
+func (m *safeMapOfStrings) get(key string) string {
+	m.mutex.RLock()
+	defer m.mutex.RUnlock()
+	return m.data[key]
+}
+
+func (m *safeMapOfStrings) set(key, value string) {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+	m.data[key] = value
+}

internal/imap/updates.go

@@ -235,7 +235,7 @@ func (iu *imapUpdates) getIDs(address, mailboxName string) (addressID, mailboxID
 	}
 	addressID = user.addressID
 
-	if v := user.mailboxIDs[mailboxName]; v != "" {
+	if v := user.mailboxIDs.get(mailboxName); v != "" {
 		mailboxID = v
 	}
 

internal/imap/user.go

@@ -54,8 +54,8 @@ type imapUser struct {
 	// UNSELECT, CLOSE, or LOGOUT.
 	appendExpungeLock sync.Mutex
 
-	addressID  string            // cached value for logs to avoid lock
+	addressID  string           // cached value for logs to avoid lock
-	mailboxIDs map[string]string // cached values for logs to avoid lock
+	mailboxIDs safeMapOfStrings // cached values for logs to avoid lock
 }
 
 // newIMAPUser returns struct implementing go-imap/user interface.
@@ -88,7 +88,7 @@ func newIMAPUser(
 
 		currentAddressLowercase: strings.ToLower(address),
 		addressID:               addressID,
-		mailboxIDs:              map[string]string{},
+		mailboxIDs:              newSafeMapOfString(),
 	}, err
 }
 
@@ -133,7 +133,7 @@ func (iu *imapUser) ListMailboxes(showOnlySubcribed bool) ([]goIMAPBackend.Mailb
 
 	mailboxes := []goIMAPBackend.Mailbox{}
 	for _, storeMailbox := range iu.storeAddress.ListMailboxes() {
-		iu.mailboxIDs[storeMailbox.Name()] = storeMailbox.LabelID()
+		iu.mailboxIDs.set(storeMailbox.Name(), storeMailbox.LabelID())
 
 		if storeMailbox.LabelID() == pmapi.AllMailLabel && !iu.backend.bridge.IsAllMailVisible() {
 			continue

test/fakeapi/controller_control.go

@@ -80,8 +80,10 @@ func (ctl *Controller) AddUserLabel(username string, label *pmapi.Label) error {
 		ctl.labelsByUsername[username] = []*pmapi.Label{}
 	}
 
+	userLabels := ctl.labelsByUsername[username]
+
 	labelName := getLabelNameWithoutPrefix(label.Name)
-	for _, existingLabel := range ctl.labelsByUsername[username] {
+	for _, existingLabel := range userLabels {
 		if existingLabel.Name == labelName {
 			return fmt.Errorf("folder or label %s already exists", label.Name)
 		}
@@ -97,7 +99,9 @@ func (ctl *Controller) AddUserLabel(username string, label *pmapi.Label) error {
 	if label.Path == "" {
 		label.Path = label.Name
 	}
-	ctl.labelsByUsername[username] = append(ctl.labelsByUsername[username], label)
+	userLabels = append(userLabels, label)
+
+	ctl.labelsByUsername[username] = userLabels
 	ctl.resetUsers()
 	return nil
 }

test/fakeapi/labels.go

@@ -88,7 +88,7 @@ func (api *FakePMAPI) listLabels(_ context.Context, labeType string, route strin
 	if err := api.checkAndRecordCall(GET, route+"/"+labeType, nil); err != nil {
 		return nil, err
 	}
-	return api.labels, nil
+	return append([]*pmapi.Label{}, api.labels...), nil
 }
 
 func (api *FakePMAPI) createLabel(_ context.Context, label *pmapi.Label, route string) (*pmapi.Label, error) {

test/features/imap/mailbox/list.feature

@@ -38,6 +38,97 @@ Feature: IMAP list mailboxes
     Then IMAP response contains "Trash"
     Then IMAP response contains "All Mail"
 
+  Scenario: List multiple times in parallel without crash
+    Given there is "user" with mailboxes
+      | Folders/mbox1  |
+      | Folders/mbox2  |
+      | Folders/mbox3  |
+      | Folders/mbox4  |
+      | Folders/mbox5  |
+      | Folders/mbox6  |
+      | Folders/mbox7  |
+      | Folders/mbox8  |
+      | Folders/mbox9  |
+      | Folders/mbox10 |
+      | Folders/mbox11 |
+      | Folders/mbox12 |
+      | Folders/mbox13 |
+      | Folders/mbox14 |
+      | Folders/mbox15 |
+      | Folders/mbox16 |
+      | Folders/mbox17 |
+      | Folders/mbox18 |
+      | Folders/mbox19 |
+      | Folders/mbox20 |
+      | Labels/lab1  |
+      | Labels/lab2  |
+      | Labels/lab3  |
+      | Labels/lab4  |
+      | Labels/lab5  |
+      | Labels/lab6  |
+      | Labels/lab7  |
+      | Labels/lab8  |
+      | Labels/lab9  |
+      | Labels/lab10 |
+      | Labels/lab11 |
+      | Labels/lab12 |
+      | Labels/lab13 |
+      | Labels/lab14 |
+      | Labels/lab15 |
+      | Labels/lab16 |
+      | Labels/lab17 |
+      | Labels/lab18 |
+      | Labels/lab19 |
+      | Labels/lab20 |
+      | Labels/lab1.1  |
+      | Labels/lab1.2  |
+      | Labels/lab1.3  |
+      | Labels/lab1.4  |
+      | Labels/lab1.5  |
+      | Labels/lab1.6  |
+      | Labels/lab1.7  |
+      | Labels/lab1.8  |
+      | Labels/lab1.9  |
+      | Labels/lab1.10 |
+      | Labels/lab1.11 |
+      | Labels/lab1.12 |
+      | Labels/lab1.13 |
+      | Labels/lab1.14 |
+      | Labels/lab1.15 |
+      | Labels/lab1.16 |
+      | Labels/lab1.17 |
+      | Labels/lab1.18 |
+      | Labels/lab1.19 |
+      | Labels/lab1.20 |
+      | Labels/lab2.1  |
+      | Labels/lab2.2  |
+      | Labels/lab2.3  |
+      | Labels/lab2.4  |
+      | Labels/lab2.5  |
+      | Labels/lab2.6  |
+      | Labels/lab2.7  |
+      | Labels/lab2.8  |
+      | Labels/lab2.9  |
+      | Labels/lab2.10 |
+      | Labels/lab2.11 |
+      | Labels/lab2.12 |
+      | Labels/lab2.13 |
+      | Labels/lab2.14 |
+      | Labels/lab2.15 |
+      | Labels/lab2.16 |
+      | Labels/lab2.17 |
+      | Labels/lab2.18 |
+      | Labels/lab2.19 |
+      | Labels/lab2.20 |
+    And there is IMAP client "A" logged in as "user"
+    And there is IMAP client "B" logged in as "user"
+    When  IMAP client "A" lists mailboxes
+    And  IMAP client "B" lists mailboxes
+    Then IMAP response to "A" is "OK"
+    And IMAP response to "A" contains "mbox1"
+    And IMAP response to "A" contains "mbox10"
+    And IMAP response to "A" contains "mbox20"
+
   @ignore-live
   Scenario: List mailboxes with subfolders
     # Escaped slash in the name contains slash in the name.

test/imap_actions_mailbox_test.go

@@ -28,6 +28,7 @@ func IMAPActionsMailboxFeatureContext(s *godog.ScenarioContext) {
 	s.Step(`^IMAP client renames mailbox "([^"]*)" to "([^"]*)"$`, imapClientRenamesMailboxTo)
 	s.Step(`^IMAP client deletes mailbox "([^"]*)"$`, imapClientDeletesMailbox)
 	s.Step(`^IMAP client lists mailboxes$`, imapClientListsMailboxes)
+	s.Step(`^IMAP client "([^"]*)" lists mailboxes$`, imapClientNamedListsMailboxes)
 	s.Step(`^IMAP client selects "([^"]*)"$`, imapClientSelects)
 	s.Step(`^IMAP client gets info of "([^"]*)"$`, imapClientGetsInfoOf)
 	s.Step(`^IMAP client "([^"]*)" gets info of "([^"]*)"$`, imapClientNamedGetsInfoOf)
@@ -63,8 +64,12 @@ func imapClientDeletesMailbox(mailboxName string) error {
 }
 
 func imapClientListsMailboxes() error {
-	res := ctx.GetIMAPClient("imap").ListMailboxes()
+	return imapClientNamedListsMailboxes("imap")
-	ctx.SetIMAPLastResponse("imap", res)
+}
+
+func imapClientNamedListsMailboxes(clientName string) error {
+	res := ctx.GetIMAPClient(clientName).ListMailboxes()
+	ctx.SetIMAPLastResponse(clientName, res)
 	return nil
 }