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

[Tanu Kaskinen]

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
> > > > > 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?

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.

-- 
Tanu

https://www.patreon.com/tanuk