xow icon indicating copy to clipboard operation
xow copied to clipboard

Incorrect thread context

Open kakra opened this issue 5 years ago • 2 comments

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.

kakra avatar Jun 29 '20 07:06 kakra

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.

medusalix avatar Jun 29 '20 10:06 medusalix

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. :-)

kakra avatar Jun 29 '20 11:06 kakra