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
