~sircmpwn/aerc

maildir: fix race condition in openDirectory v1 PROPOSED

wagner riffel: 2
 maildir: fix race condition in openDirectory
 maildir: fix race condition in openDirectory

 2 files changed, 19 insertions(+), 3 deletions(-)
Hey Reto, friendly pinging, in case you missed my reply, it's been
over 15 days since your last reply and I'm willing to work on changes
to get this merged.
Best Regards

--wagner
Hallo Wagner,

On Mon, Apr 26, 2021 at 12:58:47AM -0300, wagner riffel wrote:
Next
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~sircmpwn/aerc/patches/21933/mbox | git am -3
Learn more about email & git
View this thread in the archives

[PATCH] maildir: fix race condition in openDirectory Export this patch

Signed-off-by: wagner riffel <w@104d.net>
---
This race condition happens often while aerc is openning a maildir
directory, deleteing this second PostMessage calls also works, but
right above there is a comment it has to be sent twice for some
unknown reason, so i left the second send but using a copy of info
instead.
 worker/maildir/worker.go | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index 4a7ae51..094856c 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -304,7 +304,15 @@ func (w *Worker) handleOpenDirectory(msg *types.OpenDirectory) error {
		Info: w.getDirectoryInfo(msg.Directory),
	}
	w.worker.PostMessage(info, nil)
	w.worker.PostMessage(info, nil)

	// NOTE: this is a copy of info so we don't post the same *Message twice
	// into the queue causing a race conditions within worker.ProcessMessage
	// (called from main goroutine) and worker.PostMessage (called from
	// worker.Run() goroutine)
	info2 := &types.DirectoryInfo{
		Info: info.Info,
	}
	w.worker.PostMessage(info2, nil)
	return nil
}

-- 
2.30.2
Hi,

On Fri, Apr 09, 2021 at 04:37:32PM -0300, wagner riffel wrote:

Re: [PATCH] maildir: fix race condition in openDirectory Export this patch

On Mon Apr 12, 2021 at 1:54 PM -03, Reto wrote:
> Do you want to have a look? I'd appreciate it.
>

Here's what I could dig when I was investigating this:

We send the first message here from the worker goroutine:
https://git.sr.ht/~sircmpwn/aerc/tree/a5553438/item/worker/maildir/worker.go#L306

Then Tick() is waked in the main goroutine and calls ProcessMessage:
https://git.sr.ht/~sircmpwn/aerc/tree/a5553438/item/widgets/account.go#L100

ProcessMessage in the main goroutine reads types.Message state with
msg.getId() and msg.InResponseTo():
https://git.sr.ht/~sircmpwn/aerc/tree/a5553438/item/worker/types/worker.go#L74-76

Meanwhile in the worker goroutine we call PostMessage for a second
time with a pointer that points to the *same* previous message that
ProcessMessage is reading:
https://git.sr.ht/~sircmpwn/aerc/tree/a5553438/item/worker/maildir/worker.go#L306

The second PostMessage call makes writes to message while
ProcessMessage in the main goroutine is possibly reading:
https://git.sr.ht/~sircmpwn/aerc/tree/a5553438/item/worker/types/worker.go#L59

I didn't know it's was happening with other workers, I made a few
tests with imap and never saw such race condition, so to preserve the
same performance/behaviour across workers I thought a little copying
in maildir would be an okay fix but apparently the correct fix is to
make types.Message thread-safe, which also fixes the problem but does
locking for all workers, even those that doesn't cause this race to
happen (such as imap), how do you feel about this diff? If you're ok
with this one, I can send a proper git formatted patch

diff --git a/worker/types/messages.go b/worker/types/messages.go
index ab0e545..6858b2e 100644
--- a/worker/types/messages.go
+++ b/worker/types/messages.go
@@ -2,6 +2,7 @@ package types

import (
	"io"
	"sync"
	"time"

	"git.sr.ht/~sircmpwn/aerc/config"
@@ -15,6 +16,7 @@ type WorkerMessage interface {
}

type Message struct {
	mu           sync.Mutex
	inResponseTo WorkerMessage
	id           int64
}
@@ -25,15 +27,21 @@ func RespondTo(msg WorkerMessage) Message {
	}
}

func (m Message) InResponseTo() WorkerMessage {
func (m *Message) InResponseTo() WorkerMessage {
	m.mu.Lock()
	defer m.mu.Unlock()
	return m.inResponseTo
}

func (m Message) getId() int64 {
func (m *Message) getId() int64 {
	m.mu.Lock()
	defer m.mu.Unlock()
	return m.id
}

func (m *Message) setId(id int64) {
	m.mu.Lock()
	defer m.mu.Unlock()
	m.id = id
}

ps: In attachments there's a stack trace from the Go race detector.

-wagner
Hey Reto, friendly pinging, in case you missed my reply, it's been
over 15 days since your last reply and I'm willing to work on changes
to get this merged.

Best Regards

--wagner