medusalix / xow

Linux driver for the Xbox One wireless dongle
https://medusalix.github.io/xow
GNU General Public License v2.0
1.42k stars 91 forks source link

Incorrect thread context #101

Open kakra opened 4 years ago

kakra commented 4 years ago

https://github.com/medusalix/xow/blob/4aa49f27cb6fcb3da995da9e8d51167bed40f520/dongle/dongle.cpp#L169

This line may actually be called from two different threads because it's indirectly called from one of the two USB endpoint threads. This may introduce serialization issues in the called class as the logic there isn't prepared to be running in two different thread contexts. The dongle class should properly serialize all the packets it handles and send them to the controller class from one single thread only.

This could be done by wrapping handlePacket() in a loop with a condition variable and waiting for signals from the two endpoint threads. The endpoint script would put the packets on a serialized queue, the looping thread would just drain the queue when signalled, then wait again. This could use a similar approach as I did in the triple buffer implementation when waiting for the front buffer swap.

I see that you have a lock guard in place:

std::lock_guard<std::mutex> lock(controllerMutex);

While this synchronizes the threads at this point, it still calls into the controller class from two different threads. If the controller class uses asynchronous functions in this code path, you may end up with unexpected results or unexpected side effects.

medusalix commented 4 years ago

If the controller class uses asynchronous functions in this code path, you may end up with unexpected results or unexpected side effects.

It creates a thread for the input device in that code path, will that cause issues? Apart from that it's basically only synchronously reading the packets and transmitting data back to the controller.

kakra commented 4 years ago

Currently it will probably cause no issues but following the principle of least surprise it may be worth fixing. I may look at that later and suggest a PR, no need to bother with it just right now. Let's just keep that open as a hint for future surprises until then. :-)