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

[Tanu Kaskinen]

On Fri, 2012-03-30 at 13:33 +0300, Tanu Kaskinen wrote:
> 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.

After thinking a bit more about this, I'd say deprecating the function
is not needed. The wakeup_pipe check can be removed, because the fd is
guaranteed to be valid as long as the pa_mainloop instance exists (and
if this function is called after freeing the mainloop, there are bigger
problems than writing to an invalid fd). wakeup_pipe value never
changes, so it's ok to read it from multiple threads. The state check
can be safely removed also.

Reading and writing wakeup_pipe_type from multiple threads is still a
bit ugly, but acceptable. It can only ever be 0 or 1, so there are no
atomicity problems. If it's possible that caching can cause an old value
passed to pa_write(), it's not a big issue either, because the pipe type
seems to be just an optimization. Things should always work correctly
with either 0 or 1.

I'll prepare some patches.

-- 
Tanu