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

feat: Notification monitoring system #21

Merged
merged 14 commits into from
Jul 14, 2023
Merged

feat: Notification monitoring system #21

merged 14 commits into from
Jul 14, 2023

Conversation

realvarx
Copy link
Contributor

@realvarx realvarx commented Jul 13, 2023

- What I did
Implement AtMonitorConnection and notification monitoring system.

- How I did it

  • Took AtMonitorConnection.java as an example and implemented it, but used locks instead of synchronized (because synchronized doesn't exist in Python, it needs to be created as a decorator).

  • Created AtEventType enum and implemented an event system based on Queue library. There were EventBus solutions based on asyncio lib, but I didn't find practical the added complexity of mixing Threading and asyncio for now.

  • Created AtEvent class, which is essentially a wrapper for event + data associated to the event. AtEvent objects are then introduced into the Queue and read by other thread which will use that data.

  • Added new parameters which will be eventually necessary to Metadata class (although they are commented because there's a circular import issue).

  • TimeUtil to manage milliseconds, SocketUtil to create readline() method for SSLSocket.

  • Created AtNotification class, although it isn't used for now.

  • Added start_monitor and stop_monitor methods to AtClient class, which can be used to start the monitoring for notifications. Added handle_event method to AtClient class.

  • atconstants.py is simply a file to define constants or global variables, such as locks or queues.

  • Updated repl.py, which now includes a practical example of how this new feature can be used (simply create a new thread which calls a method, and handle the event reception through queues inside that method, also calling inside of it the handle_event method of AtClient class).

  • Small tweaks not mentioned, feel free to ask.

  • Issues:

  1. Metadata circular imports exception if new code is uncommented.
  2. Unexpected MONITOR_EXCEPTION events triggered when reading @atSign@notification notifications from yourself (doesn't happen with normal notification notifications also from yourself, which are actually the same as the other mentioned).
  3. Current way to manage events with handle_event methods could be improved to a more abstract structure, although they work for now.

- How to verify it

  1. Run repl.py and choose an atSign using option 1
  2. Select option 2. REPL will start and activate monitor mode automatically in a different thread. You can still send commands/verbs. You will start seeing your own notifications (from yourself to yourself) and heartbeat working (noop verb is sent from time to time as a keepalive)
  3. Use at_talk or any other tool to send notifications to your atSign from a different atSign. You should be able to see the complete notification, and the encrypted and decrypted value of it.

- Description for the changelog

feat: Notification monitoring system

Progress in #8 and #9

@realvarx realvarx added the enhancement New feature or request label Jul 13, 2023
@realvarx realvarx requested review from cpswan and shahumang19 July 13, 2023 09:22
@realvarx realvarx self-assigned this Jul 13, 2023
Copy link
Member

@cpswan cpswan left a comment

Choose a reason for hiding this comment

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

Please remove all the commented code from metadata.py

The README should also be updated to provide an example of receiving a notification. It would be good to have some example code too, and some tests.

at_client/common/metadata.py Outdated Show resolved Hide resolved
@cpswan
Copy link
Member

cpswan commented Jul 13, 2023

Essentially the stuff above in the How to verify it section should be finding its way into more obvious docs (like the README) rather than being buried in this PR.

cpswan
cpswan previously approved these changes Jul 13, 2023
alvaro gave me the thumbs up
Copy link
Member

@Xlin123 Xlin123 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpswan cpswan merged commit c93c9f7 into trunk Jul 14, 2023
@cpswan cpswan deleted the realvarx-dev branch July 14, 2023 10:08
This was referenced Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants