-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
moved retryCount before validating the session #2167
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a performance optimization to improve CPU utilization by moving the retryCount > 1 check before expensive session validation operations, and fixes a resource leak in the event buffer system that was scheduling thousands of redundant flush timeouts.
Key changes:
- Moved retry count validation from inside
shouldRecreateSession()to the call sites, avoiding unnecessary session validation whenretryCount <= 1 - Added
flushPendingTimeouttracking to prevent scheduling multiple flush timeouts in the event buffer - Modified event buffer's
bufferCountinitialization and increment logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Utils/message-retry-manager.ts | Removed retryCount parameter and internal retry count check from shouldRecreateSession(), simplifying the method to only handle session existence and recreation timing |
| src/Socket/messages-recv.ts | Added retryCount > 1 guard before calling session validation logic, moved retry count check upstream to avoid expensive operations; updated optional chaining for safer property access |
| src/Utils/event-buffer.ts | Added flushPendingTimeout variable to prevent scheduling multiple flush timeouts; modified bufferCount initialization to reset on new buffering session |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the work 👍 |
|
Wouldn't it be good to open another PR refactoring the entire code to improve maintainability and reduce complexity? |
somebody that is more familiar with the library should take care of that |
purpshell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be rewritten. I don't see any immediate issues from the code you have suggested. Pending testing before merge
yeah I will either work on this myself or assign it to someone like @vinikjkkj or @jlucaso1 The code from @pvictorlv is totally horrible. I mean that in the best way possible. I will gut it all out and build retries step by step from scratch following the mechanisms of whatsmeow and whatsapp web |
that will 100% fix the memeory leak issues |
|
@YonkoSam i trying revert this commit and fix compatibility |
vinikjkkj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you sent event-buffer changes from your other PR here
4d50f96 to
8ff02c4
Compare
fixed |
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
to be honest all this code should be rewritten, but for now, this will drastically improve CPU utilization.