[pulseaudio-discuss] [PATCH v0 01/16] sink, source: Fix missing pa_asyncmsgq_ref()

[Mikel Astiz]

Hi Tanu,

On Sun, Sep 30, 2012 at 9:55 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Fri, 2012-09-28 at 17:45 +0200, Mikel Astiz wrote:
>> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>>
>> Sinks and sources hold a pointer to asyncmsgq, so they should use the
>> reference counting mechanism to make sure this queue is not freed.
>
> I don't really want to apply this. This shouldn't be needed, so if this
> makes some bug unreproducible, it's a case of hiding a programming
> error, not really fixing it.

As a side comment, note that the rest of the patches in this patchset
do not depend on this one, as explained in the cover letter.

>
> Explanation why this shouldn't be needed:
>
> The pa_sink.asyncmsgq variable should only be used by the main thread,
> and only when the sink is linked. pa_sink.asyncmsgq always points to
> pa_thread_mq's inq. pa_thread_mq.inq is created in pa_thread_mq_init()
> and unreffed in pa_thread_mq_done(). pa_thread_mq_done() is only called
> after the "hardware sink" (the one implementing the IO thread) has been
> unlinked. Therefore, at least the hardware sink is fine without any

This was exactly the condition that was not met during my testing with
the bluetooth modules. As checked in patch v0 11/16, a potential error
in start_thread() could lead to this inconsistent state where the
sinks/sources were not unlinked (until I realized patch v0 07/16 was
needed). This was obviously a bug, but such reference counting would
have come handy to debug the problem.

> extra pa_asyncmsgq_ref() calls. What about filter sinks then? Can they
> be linked while the hardware sink is not, therefore maybe allowing the
> filter sink's asyncmsgq to be used while the underlying pa_thread_mq is
> gone? No. The filter sinks are unlinked at the same time when the
> hardware sink is unlinked. So, pa_sink.asyncmsgq will always have a
> reference when it's needed. No need to call pa_asyncmsgq_ref() in
> pa_sink_set_asyncmsgq().

What about checking the reference counter pa_thread_mq_done()? If I
got this right, we would expect that the reference count is 1 at that
point, so we could add an assertion. If there is some sink/source that
hasn't been unlinked, the assertion would fail. Would this check be
valid for the rest of the code using pa_asyncmsgq_ref()?

Cheers,
Mikel