diff --git a/internal/imap/mailbox.go b/internal/imap/mailbox.go index eea2fead..d8d6203b 100644 --- a/internal/imap/mailbox.go +++ b/internal/imap/mailbox.go @@ -177,16 +177,11 @@ func (im *imapMailbox) Check() error { // Expunge permanently removes all messages that have the \Deleted flag set // from the currently selected mailbox. func (im *imapMailbox) Expunge() error { - // Wait for any APPENDS to finish in order to avoid data loss when - // Outlook sends commands too quickly STORE \Deleted, APPEND, EXPUNGE, - // APPEND FINISHED: - // - // Based on Outlook APPEND request we will not create new message but - // move the original to desired mailbox. If the message is currently - // in Trash or Spam and EXPUNGE happens before APPEND processing is - // finished the message is deleted from Proton instead of moved to - // the desired mailbox. - im.user.waitForAppend() + // See comment of appendExpungeLock. + if im.storeMailbox.LabelID() == pmapi.TrashLabel || im.storeMailbox.LabelID() == pmapi.SpamLabel { + im.user.appendExpungeLock.Lock() + defer im.user.appendExpungeLock.Unlock() + } im.user.backend.updates.block(im.user.currentAddressLowercase, im.name, operationDeleteMessage) defer im.user.backend.updates.unblock(im.user.currentAddressLowercase, im.name, operationDeleteMessage) @@ -197,6 +192,12 @@ func (im *imapMailbox) Expunge() error { // UIDExpunge permanently removes messages that have the \Deleted flag set // and UID passed from SeqSet from the currently selected mailbox. func (im *imapMailbox) UIDExpunge(seqSet *imap.SeqSet) error { + // See comment of appendExpungeLock. + if im.storeMailbox.LabelID() == pmapi.TrashLabel || im.storeMailbox.LabelID() == pmapi.SpamLabel { + im.user.appendExpungeLock.Lock() + defer im.user.appendExpungeLock.Unlock() + } + im.user.backend.updates.block(im.user.currentAddressLowercase, im.name, operationDeleteMessage) defer im.user.backend.updates.unblock(im.user.currentAddressLowercase, im.name, operationDeleteMessage) diff --git a/internal/imap/mailbox_message.go b/internal/imap/mailbox_message.go index 42d6190b..b6a0f7f9 100644 --- a/internal/imap/mailbox_message.go +++ b/internal/imap/mailbox_message.go @@ -70,9 +70,6 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L // Called from go-imap in goroutines - we need to handle panics for each function. defer im.panicHandler.HandlePanic() - im.user.appendStarted() - defer im.user.appendFinished() - m, _, _, readers, err := message.Parse(body) if err != nil { return err @@ -154,6 +151,9 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L } } + im.user.appendExpungeLock.Lock() + defer im.user.appendExpungeLock.Unlock() + // Avoid appending a message which is already on the server. Apply the // new label instead. This always happens with Outlook (it uses APPEND // instead of COPY). diff --git a/internal/imap/user.go b/internal/imap/user.go index 0baaeef9..79d0d1fc 100644 --- a/internal/imap/user.go +++ b/internal/imap/user.go @@ -41,7 +41,22 @@ type imapUser struct { currentAddressLowercase string - appendInProcess sync.WaitGroup + // Some clients, for example Outlook, do MOVE by STORE \Deleted, APPEND, + // EXPUNGE where APPEN and EXPUNGE can go in parallel. Usual IMAP servers + // do not deduplicate messages and this it's not an issue, but for APPEND + // for PM means just assigning label. That would cause to assign label and + // then delete the message, or in other words cause data loss. + // go-imap does not call CreateMessage till it gets the whole message from + // IMAP client, therefore with big message, simple wait for APPEND before + // performing EXPUNGE is not enough. There has to be two-way lock. Only + // that way even if EXPUNGE is called few ms before APPEND and message + // is deleted, APPEND will not just assing label but creates the message + // again. + // The issue is only when moving message from folder which is causing + // real removal, so Trash and Spam. Those only need to use the lock to + // not cause huge slow down as EXPUNGE is implicitly called also after + // UNSELECT, CLOSE, or LOGOUT. + appendExpungeLock sync.Mutex } // newIMAPUser returns struct implementing go-imap/user interface. @@ -251,15 +266,3 @@ func (iu *imapUser) CreateMessageLimit() *uint32 { upload := uint32(maxUpload) return &upload } - -func (iu *imapUser) appendStarted() { - iu.appendInProcess.Add(1) -} - -func (iu *imapUser) appendFinished() { - iu.appendInProcess.Done() -} - -func (iu *imapUser) waitForAppend() { - iu.appendInProcess.Wait() -}