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

websocket improvements #4129

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

Conversation

austinmilt
Copy link

@austinmilt austinmilt commented Dec 1, 2024

This PR primarily recreates #3773, which you should read to understand the original contributor's (@jtigues) motivations and justifications. I have adapted some of those changes in a way that makes more sense to me. In particular, I added overloads that allow a callback function to be given that is called when response messages are received.

I realized while writing this code that my own needs are to have EmuHawk act as a websocket server (rather than as a client which this PR addresses), and so I will follow up this PR with another that serves my needs.

Main Changes

Refer to #3773 for primary changes. Below I record only those that deviate from #3773

  • added two overloads that enable a callback to be used to consume messages from the server, rather than caching messages and popping from the queue (ClientWebSocketWrapper.cs). These overloads could be used, for instance, within CommonLuaLibrary.cs to create a Lua ws_send_and_receive to automatically perform bidirectional communication with the server.
  • rather than connect and wait in the instantiation of ClientWebSocketWrapper I defer the async connection process to Connect, which better fits with its usage and avoids the awkward behavior in the constructor (ClientWebSocketWrapper.cs, WebsocketServer.cs, CommonLuaLibrary.cs)

Other Changes

  • use a FIFO queue for caching messages rather than a regular List, since the queue better matches the use case (ClientWebSocketWrapper.cs, CommonLuaLibrary.cs)

Contributor Checklist

Check if completed:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant