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.
