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

[Georg Chini]

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
>>>> BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING message. This
>>>> 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
>> pending  BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING
>> 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?