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 support (#83) #1214

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isuru89
Copy link
Contributor

@isuru89 isuru89 commented Dec 4, 2023

Technical implementation details

After a completion of successful PoC, I thought I would raise this pull request with the initial implementation of the WebSockets.

This implementation covers all basic features of a websocket. Followings are the features and limitations.

Features:

  • Can define WebSocket protocol (optionally with TLS) end points and consume them. (e.g. ws://localhost:3000/<sub-path>)
  • Can mix both WebSocket and HTTP endpoints together in the same environment.
  • Ability to respond based on the content of incoming message (full-duplex conversations)
  • Ability to broadcast same data to all the connected clients of a particular WebSocket endpoint (broadcast mode)
  • Ability to unicast streaming data for each connected client of a particular WebSocket endpoint (one-to-one mode)

Limitations:

  • No Socket.IO support. Only ws/wss portocols.
  • Path parameters are not supported for WebSockets
  • No multicast support (this feature is in socket.io with rooms). Only one-to-one and broadcast streaming is supported.
  • No authentications assumed.

Screenshots:

Adding a WebSocket route.
mockoon-ws-add

View of the WebSocket route.
mockoon-ws-routes

Shows it in logs.
mockoon-ws-logs


Checklist

  • data migration added (@mockoon/commons)
  • data migration automated tests added (@mockoon/commons)
  • CLI automated tests added (@mockoon/cli)
  • desktop automated tests added (@mockoon/desktop)

Closes #83

Copy link
Member

@255kb 255kb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this huge amount of work!

I started testing and reviewing but encountered some errors that prevented me from continuing further. I will retest as you update the PR.
Aside from the comments I already put on the code, here are some remarks:

  • I think the badge should be reused in the routes menu with a "WS" instead of an icon. Same for the logs menu:

image

  • in conversation mode, when I was modifying the body, latency, rules, etc. It wasn't applied until I restarted the server. This is not the case with the regular HTTP or CRUD routes. I don't know what is missing in the code, but you will see for the HTTP/CRUD routes a refreshEnvironment function being called before each request (

    )

  • I'm not sure if the fallback mode make sense for a persisting websocket connection. I think this option should be disabled.

image

  • I think the initial naming of the responses could be kept: "Response 1 {documentation}"

image

  • I didn't see the WS_UNKNOWN_ROUTE happening in the logs when trying to connect on the wrong route

  • when activating one of the streaming options (broadcast or one-to-one) I always got the same error message:

{"date":"Tue Jan 16 2024 10:58:16 GMT+0100 (Central European Standard Time)","error":{},"exception":true,"level":"error","message":"uncaughtException: Unexpected end of JSON input\nSyntaxError: Unexpected end of JSON input\n    at JSON.parse (<anonymous>)\n    at parseWebSocketMessage (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js:25:25)\n    at WebSocketHelpers (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js:38:55)\n    at TemplateParser (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\template-parser.js:29:103)\n    at MockoonServer.deriveFinalResponseContentForWebSockets (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js:488:60)\n    at Timeout._onTimeout (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js:511:34)\n    at listOnTimeout (node:internal/timers:569:17)\n    at process.processTimers (node:internal/timers:512:7)","os":{"loadavg":[0,0,0],"uptime":98385.875},"process":{"argv":["c:\\dev\\mockoon\\mockoon\\packages\\desktop\\node_modules\\electron\\dist\\electron.exe",".","--remote-debugging-port=8888","--data-folder=./tmp"],"cwd":"c:\\dev\\mockoon\\mockoon\\packages\\desktop","execPath":"c:\\dev\\mockoon\\mockoon\\packages\\desktop\\node_modules\\electron\\dist\\electron.exe","gid":null,"memoryUsage":{"arrayBuffers":0,"external":3718739,"heapTotal":54034432,"heapUsed":46952552,"rss":155590656},"pid":40784,"uid":null,"version":"v18.16.1"},"stack":"SyntaxError: Unexpected end of JSON input\n    at JSON.parse (<anonymous>)\n    at parseWebSocketMessage (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js:25:25)\n    at WebSocketHelpers (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js:38:55)\n    at TemplateParser (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\template-parser.js:29:103)\n    at MockoonServer.deriveFinalResponseContentForWebSockets (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js:488:60)\n    at Timeout._onTimeout (c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js:511:34)\n    at listOnTimeout (node:internal/timers:569:17)\n    at process.processTimers (node:internal/timers:512:7)","timestamp":"2024-01-16T09:58:16.377Z","trace":[{"column":null,"file":null,"function":"JSON.parse","line":null,"method":"parse","native":false},{"column":25,"file":"c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js","function":"parseWebSocketMessage","line":25,"method":null,"native":false},{"column":55,"file":"c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\templating-helpers\\web-socket-helpers.js","function":"WebSocketHelpers","line":38,"method":null,"native":false},{"column":103,"file":"c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\template-parser.js","function":"TemplateParser","line":29,"method":null,"native":false},{"column":60,"file":"c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js","function":"MockoonServer.deriveFinalResponseContentForWebSockets","line":488,"method":"deriveFinalResponseContentForWebSockets","native":false},{"column":34,"file":"c:\\dev\\mockoon\\mockoon\\packages\\commons-server\\dist\\cjs\\libs\\server\\server.js","function":"Timeout._onTimeout","line":511,"method":"_onTimeout","native":false},{"column":17,"file":"node:internal/timers","function":"listOnTimeout","line":569,"method":null,"native":false},{"column":7,"file":"node:internal/timers","function":"process.processTimers","line":512,"method":"processTimers","native":false}]}

Some questions, as I couldn't test everything:

  • in streaming mode, does the rules, random/sequential/disable rules modes work?
  • Shouldn't we put a minimum interval for the streaming mode? I'm afraid if we can setup something really small like 1ms it could crash everything.
  • I think the streaming one-to-one icon could be another one, a bit clearer: maybe this one? We already have it in the app but don't use it anymore.

Thank you. Let me know on Discord if you want to discuss some of these questions.

@isuru89 isuru89 force-pushed the feature/83-websockets branch 2 times, most recently from c3836fc to ea77b33 Compare January 27, 2024 10:11
@isuru89
Copy link
Contributor Author

isuru89 commented Feb 5, 2024

To answer your questions.

1. In streaming mode, does the rules, random/sequential/disable rules modes work?
Yes, in streaming mode, one can configure response for each emission randomly, sequentially or always with default response. Rules will be automatically disabled for streaming websockets, because rules will be useful in bidirectional communications, not for unidirectional scenarios like streaming. And I have added a message indicating it.

2. Shouldn't we put a minimum interval for the streaming mode? I'm afraid if we can setup something really small like 1ms it could crash everything.
Great, suggestion. With latest changes addressing all these review comments, I am going to have a lower bound of 10ms for streaming interval, if it is too low. Also, a warning message will be shown to user if user specified a very low value <10ms.

3. I think the streaming one-to-one icon could be another one, a bit clearer: maybe this one? We already have it in the app but don't use it anymore.
I like the icon you linked in the question and has been changed to it. The app has similar icon under repeat but it has solid arrows. I think dashed/dotted arrows would be perfect.

@isuru89 isuru89 force-pushed the feature/83-websockets branch 3 times, most recently from 7c37dbc to 8482905 Compare February 23, 2024 02:12
@isuru89 isuru89 force-pushed the feature/83-websockets branch 2 times, most recently from 482c5d4 to 7c7c35d Compare May 19, 2024 13:09
@inuitviking
Copy link

Hi!

What's needed to get this completed, @255kb and @isuru89 ?

I'm working for a company that is heavily using the forked version so far (with great success!), and would love to see this come to fruition, and make developing for and with websockets easier. It's already easier as it is now, so we're very thankful for such a contribution to such a wonderful tool! 😄

I'm not a JS dev, and have never touched electron, but I'm more than willing to test on Windows and Linux, and could provide feedback on that matter. I could look at the code as well, but I fear my lack of experience in JS and its inevitable quirks will be a greater hindrance than a help. 😅

@255kb
Copy link
Member

255kb commented Jun 7, 2024

Hi @inuitviking
Sorry for not discussing this more publicly (we discussed with @isuru89 on Discord), but after testing this PR, I think there is a major thing missing from the normal ws feature (not the broadcasting mode). When sending messages to the websocket, all the responses are based on the first message, instead of regenerating the template and interpreting the rules each time, like it is right now for the other types of routes, http and crud. I don't see any reason why the bahavior should be different for the ws type.
Last time I tested (~2 weeks ago) it was still the case.

Again, not talking about broadcasting mode that sends the same message all the time. I think it makes sense.

@isuru89 I don't think you had time to change this and update your PR?

As a side note, as this PR is modifying the data model (new route type and options) and because we now have a cloud, and because we don't force updates (on purpose), this must be released in a major version.
I would like to avoid releasing too frequent major versions like in the past, and release only one per year, maybe two max. This would be the occasion to pack other changes requiring a major version bump.

@inuitviking
Copy link

Thanks for the swift reply!

When sending messages to the websocket, all the responses are based on the first message, instead of regenerating the template and interpreting the rules each time, like it is right now for the other types of routes, http and crud. I don't see any reason why the bahavior should be different for the ws type.

That explains the odd behaviour with the websocket when testing, and it working as expected with only the first few messages after a restart of the server. Would be cool with a resolution on that matter! 😄

As a side note, as this PR is modifying the data model (new route type and options) and because we now have a cloud, and because we don't force updates (on purpose), this must be released in a major version.
I would like to avoid releasing too frequent major versions like in the past, and release only one per year, maybe two max. This would be the occasion to pack other changes requiring a major version bump.

Completely understandable! Gotta be careful.

I'll be using the fork for now, and if I find the time to look into the code, I'll hop back in here if it's needed. :)

@255kb
Copy link
Member

255kb commented Jun 7, 2024

I'll be using the fork for now, and if I find the time to look into the code, I'll hop back in here if it's needed. :)

If you can test more deeply and provide some feedback, that would be really appreciated!

@isuru89
Copy link
Contributor Author

isuru89 commented Jun 8, 2024

@255kb
I added that refresh functionality to websocket routes few weeks back. Now the template and route changes should immediately reflect in its responses.

@255kb
Copy link
Member

255kb commented Jun 8, 2024

@255kb I added that refresh functionality to websocket routes few weeks back. Now the template and route changes should immediately reflect in its responses.

Thank you.
There was this refresh thing, but also the fact that each response message should be built based on the entering message, generating a new template and re-interpreting rules like it is right now for the http routes.
I think for now, the response is always the same after the first message, even if you send a different body.
Also, I think the rules "request number" should work with ws too (each message could be counted as a request).

ws2.close();
});

it('should return correct response for each message', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@255kb
Isn't this a test case fulfilling what you mean by the other thread? Or, do you have any other different scenario?

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.

Websocket support
3 participants