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

[Tanu Kaskinen]

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.

-- 
Tanu

https://www.patreon.com/tanuk