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.

Sign up for GitHub

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

Jump to bottom

feat: enhancing protocol management with mutex locks #2137

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

feat: enhancing protocol management with mutex locks #2137

wants to merge 21 commits into from

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Sep 17, 2024 •
edited
Loading

Problem

The current implementation showcased a lot of inconsistencies around peer management for protocols.
Multiple weird behaviour were observed:

  • protocol not able to find new peers from available pool, while it truly existed
  • peer renewal for a protocol was unsuccessful about the Peer Manager count didn't reflect that
  • protocol sometimes showed a count of available peers much greater than the total actually connected peers

Solution

  • Implemented mutex locks to ensure thread-safe protocol management
  • Refactored peer management methods in the BaseProtocol class to enhance performance and reliability
  • Improved error handling and logging in the FilterCore class
  • Use async-mutex for all locks-related chores

Notes

Contribution checklist:

  • covered by unit tests;
  • covered by e2e test;
  • add ! in title if breaks public API;

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 17, 2024 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 84.52 KB (+1.15% 🔺) 1.7 s (+1.15% 🔺) 1.5 s (+7.48% 🔺) 3.2 s
Waku Simple Light Node 136.36 KB (+0.89% 🔺) 2.8 s (+0.89% 🔺) 1.8 s (-11.96% 🔽) 4.5 s
ECIES encryption 22.92 KB (-0.11% 🔽) 459 ms (-0.11% 🔽) 426 ms (-17.6% 🔽) 884 ms
Symmetric encryption 22.39 KB (-0.02% 🔽) 448 ms (-0.02% 🔽) 725 ms (+42.48% 🔺) 1.2 s
DNS discovery 72.35 KB (+0.11% 🔺) 1.5 s (+0.11% 🔺) 1.3 s (-23.47% 🔽) 2.7 s
Peer Exchange discovery 73.52 KB (-0.37% 🔽) 1.5 s (-0.37% 🔽) 1.3 s (+8.8% 🔺) 2.7 s
Local Peer Cache Discovery 67.56 KB (-0.11% 🔽) 1.4 s (-0.11% 🔽) 1.4 s (+57.91% 🔺) 2.7 s
Privacy preserving protocols 74.84 KB (+0.07% 🔺) 1.5 s (+0.07% 🔺) 1.6 s (-2.03% 🔽) 3.1 s
Waku Filter 79.28 KB (+0.97% 🔺) 1.6 s (+0.97% 🔺) 1.5 s (-17.82% 🔽) 3.1 s
Waku LightPush 77.31 KB (+0.68% 🔺) 1.6 s (+0.68% 🔺) 1.3 s (-14.55% 🔽) 2.9 s
History retrieval protocols 76.65 KB (+0.83% 🔺) 1.6 s (+0.83% 🔺) 1.4 s (-14.89% 🔽) 3 s
Deterministic Message Hashing 7.38 KB (0%) 148 ms (0%) 233 ms (-19.08% 🔽) 381 ms

@danisharora099 danisharora099 mentioned this pull request Sep 26, 2024
Merged
@danisharora099 danisharora099 changed the title chore: fix feat: enhancing protocol management with mutex locks Sep 30, 2024
@danisharora099 danisharora099 marked this pull request as ready for review October 1, 2024 13:18
@danisharora099 danisharora099 requested a review from a team as a code owner October 1, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: inconsistent protocol peer management bug: Filter subscriptions are not stable and missing messages
1 participant
@danisharora099