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

Buffer messages to wait for reconnect #697

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

loneil
Copy link
Contributor

@loneil loneil commented Dec 20, 2024

Leaving for draft for now, want to double check all retention/removal stuff so there's no memory leak possibility.

Buffer up messages that would be sent to disconnected clients. On client reconnect (just "connect" server side still) check the buffer for the pid from the client and re-emit messages.
Only keep the messages in the buffer for a fixed amount of seconds (can adjust this maybe with config if needed? Or can use the same timeout as the UI maybe since the user would be prompted to try again anyways if they leave off into the wallet app too long)

Some details on Socket.io handling disconnections:
https://socket.io/docs/v4/tutorial/handling-disconnections

Socket.io does have a built in Connection State Recovery:
https://socket.io/docs/v4/connection-state-recovery
I think this would accomplish what this code does here, however it's not implemented in the Python library VCAuth uses
see: miguelgrinberg/python-socketio#1258

Maybe we shouldn't be using this library? It's a pretty simple wrapper though but could be considerations on handling web sockets in a different way. We do however need to consider that storing in-memory is not what we want forever on VCAuth and need to be able to hook into (DB likely) persistence for multi-pod deployment. The code here should be able to handle that need when we address it.

Update the python socket library too (and other python libraries), and socket.io client JS.

@loneil loneil force-pushed the feature/queueMessages branch from 5ab0f69 to 400aad2 Compare December 20, 2024 23:23
@loneil loneil force-pushed the feature/queueMessages branch from 1514a99 to a3b0302 Compare December 21, 2024 00:13
@coveralls
Copy link

coveralls commented Dec 21, 2024

Pull Request Test Coverage Report for Build 12440513774

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.254%

Totals Coverage Status
Change from base Build 12416479364: 0.0%
Covered Lines: 688
Relevant Lines: 807

💛 - Coveralls

Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil force-pushed the feature/queueMessages branch from 43fe8eb to 687279c Compare December 21, 2024 00:36
@loneil loneil requested a review from esune December 21, 2024 00:53
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.

2 participants