Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web Socket Implementation Fixes #3773

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jtigues
Copy link

@jtigues jtigues commented Sep 13, 2023

"Fix" for the current websocket client implementation.

To preface, the existing implementation technically worked, but had a couple undesirable behaviors, as well as some missing references:

  1. If the websocket server ever lost connection while the application was still running, a number of the websocket functions would cause Bizhawk to crash, and the state was not always updated to use as a failsafe.
  2. The Receive call was not asynchronous, meaning that if you called ws_receive, and there was not a message already sent by the server, the emulator would lock up. There was also a related issue, which was that after enough times that the server tried to send a message and Bizhawk was not immediately waiting for a response, the server would just terminate the connection.

So with that said, I made the following changes:

  1. ws_open, in addition to the socket server URI, now also takes a guid (nullable), a bufferSize, and a maxMessages input. Accepting a guid means that even if an application using a websocket client crashes or needs to be restarted, the previous connection can be reused instead of the old one still sitting there while a new one spins up. Also, if the conneciton was previously established, and maybe the server just went down, it will now attempt to reconnect and start listenting again instead of the client instance being orphaned. The other new parameters come up in the next part.
  2. Changed the Receive function to be an asynchronous task that constantly loops for messages from the client, and added a List to ClientWebSocketWrapper to store messages from the server. I couldn't find a way to have the application provide the client with a function to run on message receipt, which would be ideal. So instead, I just have the server message write into a List of messages. Receive is now called via a new async task, Connect, which is called in the constructor. So the bufferSize is now handled in ws_open instead of ws_receive, and maxMessages just lets the user place a limit on how many messages to store at once. I don't expect this to get high ever, unless maybe the server is sending messages nonstop. Allowing for a value of 0 to translate into no message cap.
  3. ws_receive is reworked to return and pop the oldest message in the list
  4. Some minor bugfixes, namely some missing references, and correcting the type of status in ws_close (was expecting WebSocketCloseStatus, which Lua cannot pass as a type, so it's just casting an int now).

I've been running this, minus the maxMessages addition, for several months now without issue. specifically using StreamerBot's websocket server, and then a much less robust local server for initial testing. But happy to tweak things and make changes, of course.

Check if completed:

@YoshiRulz YoshiRulz added help wanted We'd love to have this feature, but no dev is working on it. Discussion and PRs are welcome. Needs domain knowledge for triage and removed Needs domain knowledge for triage labels Aug 5, 2024
@austinmilt
Copy link

@YoshiRulz @jtigues I'm interested in getting this one over the finish line, mostly as a way for me to figure out how to use the websockets feature - I am totally new to BizHawk development.

Is there anything I should know, or any good docs I can read to get started?

@YoshiRulz
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We'd love to have this feature, but no dev is working on it. Discussion and PRs are welcome.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants