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

Fix and isolate message receipts #116

Merged
merged 6 commits into from
Dec 16, 2021
Merged

Fix and isolate message receipts #116

merged 6 commits into from
Dec 16, 2021

Conversation

Sup3Legacy
Copy link
Contributor

This PR should both :

  • Fix Rate limiting on too many receipts #115. This issue originated in a small bug causing the client to repeatedly send empty ReceiptMessages.
  • Isolate the handling of these receipts. They take the form of ReceiptEvents that can be created anywhere in the client and they are handled in a separate structure, stored in a set (idea from @boxdot). At regular interval (default 1 per s.; this rate can safely be increased), an event, if any, is sent.

Technical clarification : the events are sent grouped by Uuid and whether they are Delivered or Read, so the number of waiting events cannot exceed 2 * (number of active conversations).

Shortcomings :

  • This handler does not get saved in the database. This means that closing the client while some receipt events are still waiting to get sent will result in them never getting sent at all.

@Sup3Legacy Sup3Legacy changed the title Receipts Fix and isolate message receipts Dec 16, 2021
Copy link
Owner

@boxdot boxdot left a comment

Choose a reason for hiding this comment

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

Very nice! Thank you. -- I have some comments.

src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/ui.rs Outdated Show resolved Hide resolved
Copy link
Owner

@boxdot boxdot 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. I just have the last two comments about the entry api, and then we can merge.

src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Show resolved Hide resolved
@boxdot
Copy link
Owner

boxdot commented Dec 16, 2021

Thank you! Awesome work.

@boxdot boxdot merged commit c12734a into boxdot:master Dec 16, 2021
@Sup3Legacy Sup3Legacy deleted the receipts branch December 16, 2021 18:01
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.

Rate limiting on too many receipts
2 participants