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

SYSTEST-9338 - Send events to the WS object that subscribed to it #122

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

ksentak
Copy link
Contributor

@ksentak ksentak commented Apr 13, 2023

Background
Mock Firebolt now supports multiple connections per user by utilizing an array/map of WS connections per userId.

This presents a problem for situations in which MF is tasked with triggering an event for a user. If a user is connected to MF with multiple connections, there is currently no way for MF to know which connection to send the requested event to.

In order to properly inject events, we must know which connection began listening to the requested event.

Implementation
MF already keeps track of which users are listening to which events. This is in order to support "sendEvent()" and "sendBroadcastEvent()" functionalities in YAML overrides as well as sending events manually using the --event CLI and its associated API. This same functionality can be extended to include which connection is listening to an event.

From there, the event functionality can be enhanced to send events to only the connections that are listening for it.

Changes Made

  • Pass WS object to registerEventListener and deregisterEventListener functions inside of messageHandler.mjs.
  • Modify registerEventListener, deregisterEventListener, & emitResponse functions inside of events.mjs.
  • registerEventHandler now takes in a ws object and it is added in the eventListenerMap.
  • When it is time to call emitResponse, the WS object is retrieved from the eventListenerMap and used to send the response.

ksentak added 2 commits April 13, 2023 11:56
WS connection will now be taken in when event is registered. It will then be used when it is time to call the emitResponse function.
@ksentak ksentak marked this pull request as ready for review April 13, 2023 16:22
kschrief
kschrief previously approved these changes Apr 14, 2023
michaelcfine
michaelcfine previously approved these changes Apr 14, 2023
Copy link
Contributor

@michaelcfine michaelcfine left a comment

Choose a reason for hiding this comment

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

Nice work! One question/comment that I don't really know the answer. Depending on your confidence with how it is now, feel free to merge. Or, if my question leads you to consider a change, just ping me again when changed.

function deregisterEventListener(userId, oMsg, ws) {
if (!eventListenerMap[userId]) { return; }

if ( eventListenerMap[userId] && eventListenerMap[userId][oMsg.method] && eventListenerMap[userId][oMsg.method].ws === ws ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log some kind of error if the ws values don't match? Would this be an unexpected error to somehow track down? Or... should we not compare ws values and nuke the listener record no matter what?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. That brings up a scenario I don't know if we thought of. What if a user is listening to the same method across multiple connections. Why they would do this I don't know but I guess it's possible.

Considering the fact that we're still limiting to one listen per user per method I'd say we just nuke it no matter what. But maybe we want to put in the feature for multiple listens per user per connection?

Keaton, what'd be the difficulty on that? I'm assuming we'd just be setting [ws] as an array and some logic on top to add/remove the proper one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per your request, I was able to set ws key to an array. I have renamed it to wsArr and we can now store multiple ws connections inside of the array.

@ksentak ksentak dismissed stale reviews from kschrief and michaelcfine via 63dd940 April 19, 2023 19:40
michaelcfine
michaelcfine previously approved these changes Apr 19, 2023
Copy link
Contributor

@michaelcfine michaelcfine left a comment

Choose a reason for hiding this comment

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

👍 good catches/fixes/improvements

Copy link
Contributor

@michaelcfine michaelcfine left a comment

Choose a reason for hiding this comment

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

Ah, good catch! Didn't think of that.

@kschrief kschrief merged commit 29a5a3a into rdkcentral:main Apr 21, 2023
@ksentak ksentak deleted the SYSTEST-9338 branch April 21, 2023 15:12
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.

3 participants