Write to websocket asynchronously, plus fix thread-safety issues #60
Conversation
|
Huh, good job! I didn't expect a PR next day, but this looks promising at first glance. As I mentioned I currently don't have any way of testing this myself and I'm not very familiar with the C#/Unity threading APIs, so I'll have to figure out some way to run it myself and read up on some documentation. I'll report back as soon as I can. |
|
I've pushed a few more minor commits that make some minor enhancements and simplify some stuff. I read a bit more about how C# event properties work and turns out that changing the listeners is already thread-safe, so the parts where I added and removed listeners only through the main thread were unnecessary, as long as the event property was safely-checked before being called which I've down now. |
|
Also, I'd like to ask that you squash the commits before I merge. |
All writes to the websocket are now async so they don't ever block the main game thread. Also the thread-safety issues were fixed where game data was being read from connection handler threads.
This fixes #3 and obsoletes #54. Additionally some thread-safety issues have been fixed so that all game-reading code happens from within the game thread. These two things have been done in separate commits. (I would have made them separate PRs except that my commit for the threading fixes depended on my async-send change.)
In https://github.com/opl-/beatsaber-http-status/issues/3#issuecomment-817007287, I theorized that the Send call inside of HTTPServer.StatusBroadcastBehavior.OnStatusChange was the cause of stuttering while using the mod. I tested this by adding some timing around the send call:
I got results like this:
Original Results
This is all within the main game thread, so the synchronous IO is regularly causing stuttering. (At 90fps, each frame has 11ms to happen, so blocking the main thread for an extra 40+ms regularly is pretty bad.) (I also did some timing around other parts of OnStatusChange and the rest of it was just zeroes.)
After the first commit, the results look perfect like this:
Fixed Results
The asynchronous send fix properly queues up writes to the websocket.
Next, I noticed that HTTPServer would directly read from the StatusManager in response to web requests and websocket events even though StatusManager was written to from the main game thread. I made the code thread-safe by making sure that all access to StatusManager happened through the game thread.
(I did consider an alternate solution: It would be possible to make it so that StatusManager.EmitStatusUpdate wrote all of its results to a single volatile property, and then HTTPServer could read this volatile property when handling requests and websocket events, but this would have been a bigger change for negligible benefit that would make the code harder to evolve. In comparison, the change to just read from StatusManager within the game thread is a very small drop-in fix.)