[pulseaudio-discuss] [PATCH v3 1/3] bluetooth: ofono: Use Acquire method if available

Georg Chini georg at chini.tk
Fri Jul 21 20:24:28 UTC 2017

On 21.07.2017 20:26, Tanu Kaskinen wrote:
> On Fri, 2017-07-21 at 20:58 +0300, Tanu Kaskinen wrote:
>> On Thu, 2017-07-20 at 18:50 +0200, Georg Chini wrote:
>>> On 20.07.2017 16:23, Tanu Kaskinen wrote:
>>>> On Tue, 2017-07-18 at 20:16 +0200, Georg Chini wrote:
>>>>> On 17.07.2017 19:44, Tanu Kaskinen wrote:
>>>>>> On Sun, 2017-07-16 at 14:35 +0200, Georg Chini wrote:
>>>>>>> On 04.07.2017 15:38, Tanu Kaskinen wrote:
>>>>>>>> It looks racy indeed, so some check should be added as you say. When
>>>>>>>> shutting down or changing the profile, stop_thread() is always called.
>>>>>>>> stop_thread() calls pa_thread_mq_done(), and this is where queued
>>>>>>>> messages are processed. The transport hasn't been released yet at this
>>>>>>>> point, but I think the transport release can be moved to happen before
>>>>>>>> pa_thread_mq_done(), then you can check if u->transport is NULL.
>>>>>>> Mh, after looking at the code, I don't see the race condition. Let's
>>>>>>> assume we went through transport_acquire() and sent a
>>>>>>> means we successfully acquired the transport. Now something
>>>>>>> happens that leads to a thread shutdown before the message is
>>>>>>> processed. This can either be a profile change or the remote end
>>>>>>> unexpectedly closing the connection. In all cases stop_thread()
>>>>>>> will be called. In stop_thread() the outstanding message will be
>>>>>>> processed and the transport set to playing in pa_thread_mq_done().
>>>>>>> This does not really matter, because the transport is released
>>>>>>> immediately after this through transport_release(). It just reflects -
>>>>>>> with a little delay - what happened in reality.
>>>>>>> In my opinion we would only have a race condition if it was possible
>>>>>>> that the transport was released before the message was processed
>>>>>>> but I do not see how this could happen.
>>>>>>> But maybe I just did not see the point (again), so please correct me
>>>>>>> if I'm wrong.
>>>>>> You have a point - I can't point to any specific thing that will
>>>>>> definitely break. However, the IO thread has already been torn down
>>>>>> when the SET_TRANSPORT_PLAYING message is processed, and I'm not sure
>>>>>> if setting the transport state to PLAYING is safe in that situation.
>>>>>> pa_bluetooth_transport_set_state() will fire some hooks, and I didn't
>>>>>> trace down what happens in those hooks. It seems safer to tear down the
>>>>>> transport while the IO thread is still running.
>>>>> After another look I understand even less ...
>>>>> Doesn't pa_thread_mq_done() process the final messages for the I/O
>>>>> thread and not for the main thread? The messages we are talking
>>>>> about are messages from the I/O thread to the main thread and are
>>>>> therefore processed in the main thread. This can't happen while the
>>>>> main thread is executing stop_thread(), so from that perspective
>>>>> it should not matter where in stop_thread() the transport is set to
>>>>> NULL (unless my assumption concerning pa_thread_mq_done() is
>>>>> wrong).
>>>>> On the other hand I think it should only be set to NULL after all I/O
>>>>> thread messages have been processed, which is after
>>>>> pa_thread_mq_done(), so I still believe releasing the transport after
>>>>> the call is correct.
>>>>> Now however I wonder if there is the possibility that there are still
>>>>> messages after stop_thread() has been called. This is should be no
>>>>> problem if pa_bluetooth_transport_set_state() just returns if the
>>>>> transport is NULL (currently it asserts).
>>>>> Do you still think I should move the transport release before
>>>>> pa_thread_mq_done()?
>>>> You're right, pa_thread_mq_done() doesn't process the messages that are
>>>> sent to the main thread, contrary to what I thought. Moving the
>>>> transport release isn't necessary.
>>>> Your suggestion of changing pa_bluetooth_transport_set_state() doesn't
>>>> seem safe: when changing profiles, u->transport will not be NULL when
>>>> pa_bluetooth_transport_set_state() is called, but the
>>>> SET_TRANSPORT_PLAYING message was meant for the old transport, not the
>>>> new one. I think the message needs to identify the transport that
>>>> should be set to PLAYING. When the message is processed, the transport
>>>> state should be set only if the current transport matches the transport
>>>> identified by the message.
>>> Because stop_thread() calls transport_release(), transport_acquired
>>> will be set to false. On the other hand, transport_acquire() sets it to
>>> true before sending the message. So would it be enough to just check
>>> if transport_acquired is still true?
>> Hmm... reading and writing transport_acquired from both threads is
>> wrong. My previous suggestion wouldn't fix that.
>> Can we just not call transport_acquire() from the IO thread? The only
>> place where that happens is in setup_transport_and_stream(), which is
>> called when the sink or source state changes to IDLE or RUNNING. Could
>> we replace the setup_transport_and_stream() call with a request from
>> the IO thread to the main thread to acquire the transport? After the
>> main thread has successfully acquired the transport, it will then have
>> to send another message to the IO thread to signal that the stream can
>> now be set up.
> I now realized that since transport_acquire() is called in the IO
> thread only when setting the sink/source state, the main loop is
> waiting, so accessing transport_acquired is safe.
> So back to considering your suggestion... So pa_transport_set_state()
> should be called from the message handler only if transport_acquired is
> true? If a profile change happens before the message is processed, and
> the new profile is not off, then transport_acquired will be true, but
> the transport will be different than what the message was intended for.
> Is your point that this doesn't matter, because even if the transport
> is "wrong", it's still correct to set the transport state to PLAYING if
> the transport is currently acquired?
Yes, exactly. During the profile switch, transport_acquire() is run from
the main thread. Now there are two possibilities:

1) transport_acquire() succeeds. In this case, the transport was already
set to PLAYING because we called pa_bluetooth_transport_set_state()
directly. The only thing that is done, when the message is processed is
that pa_bluetooth_transport_set_state() is called again with no effect
because the state is already PLAYING.

2) transport_acquire() fails. In this case, transport_acquired is not set
to true, so if we check it and don't do anything if it is false, we are 
on the safe side.

More information about the pulseaudio-discuss mailing list