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.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
