-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Per user mutex #2175
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?
Per user mutex #2175
Conversation
Hey, I've been debugging a memory leak in my application and traced it back to the makeMutex implementation. The current implementation chains promises indefinitely without ever breaking the chain: Every call to mutex() creates a new Promise that awaits the previous task, then becomes the new task. The problem is the old promises never get released because each one holds a reference to the previous through the closure. What I found Took a heap snapshot after running for a while and found hundreds of Promises from make-mutex.js holding ~15MB and growing. The retainer graph shows a long chain of Promises all pointing back to each other. Since processingMutex handles every incoming message/notification, this chain grows constantly and never shrinks. This keeps the same mutex behavior but lets the GC clean up old promises every 50 tasks instead of holding them forever.
|
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. |
…v and messages-send
aab5d8e to
8993afd
Compare
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 migrates from global message locking to per-user (per-remoteJid) locking to significantly improve performance in accounts with many concurrent chats and groups. The change introduces a new makeKeyedMutex function that maintains separate mutex instances per key, allowing operations on different users/chats to proceed in parallel while still preventing race conditions within each individual chat.
Key Changes:
- Replaced global
makeMutex()withmakeKeyedMutex()for message, receipt, and notification processing - Updated all mutex calls to include a key parameter (remoteJid or msgId)
- Refactored message handling to extract and use remoteJid and msgId as mutex keys
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Socket/chats.ts | Converted messageMutex, receiptMutex, and notificationMutex from global to keyed mutexes; updated import statement |
| src/Socket/messages-recv.ts | Added makeKeyedMutex import, converted retryMutex to keyed mutex, updated all mutex calls to pass remoteJid or msgId as key parameter |
| src/Socket/messages-send.ts | Updated messageMutex.mutex call to include jid parameter for per-user locking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (config.emitOwnEvents) { | ||
| process.nextTick(async () => { | ||
| await messageMutex.mutex(() => upsertMessage(fullMsg, 'append')) | ||
| await messageMutex.mutex(jid,() => upsertMessage(fullMsg, 'append')) |
Copilot
AI
Dec 12, 2025
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.
Missing space after comma in the mutex function call. The function call should be messageMutex.mutex(jid, () => ...) with a space after the comma for consistency with standard TypeScript formatting.
| await messageMutex.mutex(jid,() => upsertMessage(fullMsg, 'append')) | |
| await messageMutex.mutex(jid, () => upsertMessage(fullMsg, 'append')) |
| processSyncAction | ||
| } from '../Utils' | ||
| import { makeMutex } from '../Utils/make-mutex' | ||
| import { makeMutex , makeKeyedMutex} from '../Utils/make-mutex' |
Copilot
AI
Dec 12, 2025
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.
Inconsistent spacing in the import statement. There should be no space before the comma in makeMutex , makeKeyedMutex. The correct format is makeMutex, makeKeyedMutex for consistency with standard TypeScript formatting.
| import { makeMutex , makeKeyedMutex} from '../Utils/make-mutex' | |
| import { makeMutex, makeKeyedMutex } from '../Utils/make-mutex' |
| try { | ||
| await Promise.all([ | ||
| receiptMutex.mutex(async () => { | ||
| receiptMutex.mutex(remoteJid!,async () => { |
Copilot
AI
Dec 12, 2025
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.
Missing space after comma in the mutex function call. The function call should be receiptMutex.mutex(remoteJid!, async () => ...) with a space after the comma for consistency with standard TypeScript formatting.
| receiptMutex.mutex(remoteJid!,async () => { | |
| receiptMutex.mutex(remoteJid!, async () => { |
| try { | ||
| await Promise.all([ | ||
| notificationMutex.mutex(async () => { | ||
| notificationMutex.mutex(remoteJid!,async () => { |
Copilot
AI
Dec 12, 2025
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.
Missing space after comma in the mutex function call. The function call should be notificationMutex.mutex(remoteJid!, async () => ...) with a space after the comma for consistency with standard TypeScript formatting.
| notificationMutex.mutex(remoteJid!,async () => { | |
| notificationMutex.mutex(remoteJid!, async () => { |
|
|
||
| try { | ||
| await messageMutex.mutex(async () => { | ||
| await messageMutex.mutex(remoteJid,async () => { |
Copilot
AI
Dec 12, 2025
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.
Missing space after comma in the mutex function call. The function call should be messageMutex.mutex(remoteJid, async () => ...) with a space after the comma for consistency with standard TypeScript formatting.
| await messageMutex.mutex(remoteJid,async () => { | |
| await messageMutex.mutex(remoteJid, async () => { |
|
|
||
| // Handle both pre-key and normal retries in single mutex | ||
| await retryMutex.mutex(async () => { | ||
| await retryMutex.mutex(msgId,async () => { |
Copilot
AI
Dec 12, 2025
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.
Missing space after comma in the mutex function call. The function call should be retryMutex.mutex(msgId, async () => ...) with a space after the comma for consistency with standard TypeScript formatting.
| await retryMutex.mutex(msgId,async () => { | |
| await retryMutex.mutex(msgId, async () => { |
| const remoteJid = msg.key.remoteJid!; | ||
| const msgId = msg.key.id!; |
Copilot
AI
Dec 12, 2025
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.
Using the non-null assertion operator (!) on lines 1211-1212 but then checking for null/undefined on line 1214 is contradictory. Either remove the non-null assertions and use optional chaining (e.g., const remoteJid = msg.key.remoteJid;), or remove the null checks on line 1214. The original code used optional chaining to safely check existence before use. If these values can be null/undefined, the non-null assertion could cause runtime errors.
| const remoteJid = msg.key.remoteJid!; | |
| const msgId = msg.key.id!; | |
| const remoteJid = msg.key.remoteJid; | |
| const msgId = msg.key.id; |
| const remoteJid = msg.key.remoteJid!; | ||
| const msgId = msg.key.id!; | ||
|
|
||
| if (messageRetryManager && remoteJid && msgId ) { |
Copilot
AI
Dec 12, 2025
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.
Extra space before the closing parenthesis. The condition should be if (messageRetryManager && remoteJid && msgId) without a space before the closing parenthesis, for consistency with standard TypeScript formatting.
| if (messageRetryManager && remoteJid && msgId ) { | |
| if (messageRetryManager && remoteJid && msgId) { |
src/Socket/messages-recv.ts
Outdated
| xmppSignedPreKey | ||
| } from '../Utils' | ||
| import { makeMutex } from '../Utils/make-mutex' | ||
| import { makeKeyedMutex, makeMutex } from '../Utils/make-mutex' |
Copilot
AI
Dec 12, 2025
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.
Unused import makeMutex.
| import { makeKeyedMutex, makeMutex } from '../Utils/make-mutex' | |
| import { makeKeyedMutex } from '../Utils/make-mutex' |
7783603 to
c86b623
Compare
|
All that's missing is the lint to make it 100%. |
|
@jlucaso1 running on bartender, will report with any interesting findings |
|
Will you apply the copilot changes? And the lint still needs fixing 😄 |
|
The bartender PR does not produce meaningful changes. I think its cuz we are the same sender. It would require like 10-20 senders on bartender I guess to make impact. @jlucaso1 do I create 10 mock senders on there? Not sure if it would be difficult or not |
|
Just tested on multiple senders, this PR is potentially a risk. More work needs to be done in this field. Converting PR to draft |
What are the risks and what needs to be improved? |
|
@purpshell Logically, this is a huge performance boost from O(n) to O(1), but I might be wrong. |
|
@YonkoSam, that's a good suggestion. I think we can go further and implement user-specific blocking only on the signal adapter, instead of high-level blocking. We can increase the transfer rate in different chats. I'm talking with @purpshell about so many mutex and locks in the project, some are unecessary and add overhead. |
|
@jlucaso1 Yes, the mutex was causing high memory usage and eventually high CPU usage (when GC kicks in), especially with the old mutex implementation. |
|
What do you think about this pull request? I closed it because @purpshell mentioned it introduces race conditions, although I’m running it in production right now and it’s working fine. |
|
@YonkoSam something broken the stress test and is causing messages not being delivered, we don't know the cause yet. You can reopen for further investigation, no problem. |
|
I think we should also try this in real production with high instances. |
|
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 |
Right now, message locking was global, which slowed things down a lot for accounts with many groups and chats. This PR changes it so that each user (remoteJid) has its own lock.
Files updated: chats.ts, messages-recv.ts, messages-send.ts
Performance improvement is huge up to 100x faster in heavy accounts