Skip to content

Conversation

@roznawsk
Copy link
Member

@roznawsk roznawsk commented Sep 22, 2023

Implemented Server Notifications using websockets and asyncio.

I recommend taking a look at the example in the Readme first.
The notification handling occurs as following:

  1. Create a Notifier instance
  2. Define one or more event handlers using the on_server_notification and on_metrics decorators. Now the Notifier has designated handler for the given message types.
  3. Start the notifier (notifier.connect()). This opens a websocket, sends auth request and send subscribe requests but only for the event types which have declared message handlers. This also means that you cannot subscribe to an event type (notifications/ metrics) once the Notifier has started.
  4. The event loop runs forever and ends only when the connection is closed from the server or by canceling the notifier task

If you are unfamiliar with asyncio, I recommend taking a look at their docs, for example here: https://docs.python.org/3/library/asyncio-task.html

INB4: I plan to add secure option to both RoomApi and Notifier but not in this PR

@roznawsk roznawsk requested review from LVala and sgfn September 25, 2023 13:42
@roznawsk roznawsk marked this pull request as ready for review September 25, 2023 13:42
Copy link
Member

@sgfn sgfn left a comment

Choose a reason for hiding this comment

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

Looks good -- I'm not too familiar with the notifications flow, but as far as I can tell, everything is as it should be. A couple nitpicks only

README.md Outdated
```

You can use it to interact with Jellyfish managing rooms, peers and components
You can use it to interact with Jellyfish, managing rooms, peers and components
Copy link

Choose a reason for hiding this comment

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

Suggested change
You can use it to interact with Jellyfish, managing rooms, peers and components
You can use it to interact with Jellyfish, manage rooms, peers and components

or ...Jellyfish, enabling the management of rooms..., now it sounds kinda weird.

Comment on lines 61 to 62
async with client.connect(f'ws://{self._server_address}/socket/server/websocket') \
as websocket:
Copy link

Choose a reason for hiding this comment

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

Nitpick, but I don't like the line break

Suggested change
async with client.connect(f'ws://{self._server_address}/socket/server/websocket') \
as websocket:
address = f'ws://{self._server_address}/socket/server/websocket'
async with client.connect(address) as websocket:

Comment on lines +67 to +72
if self._notification_handler:
await self._subscribe_event(
event=ServerMessageEventType.EVENT_TYPE_SERVER_NOTIFICATION)

if self._metrics_handler:
await self._subscribe_event(event=ServerMessageEventType.EVENT_TYPE_METRICS)
Copy link

Choose a reason for hiding this comment

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

Does that mean that you cannot add handler after you have been connected? If so, I would mention it somewhere.

@sgfn sgfn self-requested a review September 27, 2023 07:59
Copy link
Member

@sgfn sgfn left a comment

Choose a reason for hiding this comment

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

LGTM, cool and good family-friendly version

@roznawsk roznawsk changed the title Server notifications [RTC-365] Server notifications Sep 27, 2023
@roznawsk roznawsk merged commit 7749bdb into main Sep 27, 2023
@roznawsk roznawsk deleted the server-notifications branch September 27, 2023 13:46
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.

4 participants