Hi,
The documentation for pa_mainloop_wakeup() says: "Interrupt a running
poll (for threaded systems)"
Indeed, the function is only useful for waking up the mainloop from
other threads. But is this implementation really thread-safe?
void pa_mainloop_wakeup(pa_mainloop *m) {
char c = 'W';
pa_assert(m);
if (m->wakeup_pipe[1] >= 0 && m->state == STATE_POLLING) {
pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
m->wakeup_requested++;
}
}
The code reads m->wakeup_pipe[1] and m->state, which are not volatile or
atomic. Both the variables have usually so small values that there
probably aren't any atomicity problems, but I'm wondering about caching
issues; are they guaranteed to be up-to-date? But even if the data is
up-to-date in the if condition, when this code is called from a foreign
thread, I don't think there's any guarantee that m->wakeup_pipe[1] will
be valid or that m->state is POLLING anymore when pa_write() is called
(the return value of pa_write() isn't checked, by the way, which was
flagged by Coverity and which is why I'm reading this code).
Also, incrementing a boolean variable (wakeup_requested) seems wrong.
Does a one-bit variable wrap from 1 to 0 when it's incremented? Just
setting it to TRUE would be enough, because the variable is not really
used as a counter.
The only uses of pa_mainloop_wakeup() in Pulseaudio codebase are from
mainloop.c itself. The usage suggests that Lennart meant e.g.
mainloop_defer_new() (which is one caller of pa_mainloop_wakeup()) to be
callable from multiple threads, but that for sure will lead to random
crashes (been there, done that).
Those internal pa_mainloop_wakeup() calls can be removed, but this
function happens to be a part of the public API. I'd like to declare the
function deprecated. I think applications should create the wakeup pipe
themselves and create an IO event for it if they want to wake up the
mainloop from a different thread.
--
Tanu
