On Fri, 2017-07-21 at 22:24 +0200, Georg Chini wrote:
> 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
> > > > > > > > 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.
> > 
> > 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 
> again
> on the safe side.

Ok, let's do it like that.

-- 
Tanu

https://www.patreon.com/tanuk
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Reply via email to