[pulseaudio-discuss] pa_mainloop_wakeup() broken and useless?

[Tanu Kaskinen]

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