-
Notifications
You must be signed in to change notification settings - Fork 997
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
[backend] Improve application stopping time (#9740) #9715
base: master
Are you sure you want to change the base?
Conversation
cb2dfd3
to
27a7f9e
Compare
27a7f9e
to
4b59b35
Compare
@@ -157,7 +157,7 @@ const processNotificationEvent = async ( | |||
const notifierMap = new Map(notifiers.map((n) => [n.internal_id, n])); | |||
for (let notifierIndex = 0; notifierIndex < userNotifiers.length; notifierIndex += 1) { | |||
const notifier = userNotifiers[notifierIndex]; | |||
internalProcessNotification(context, settings, notificationMap, user, notifierMap.get(notifier) ?? {} as BasicStoreEntityNotifier, data, notification); | |||
await internalProcessNotification(context, settings, notificationMap, user, notifierMap.get(notifier) ?? {} as BasicStoreEntityNotifier, data, notification); |
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.
We can maybe put all the internalProcessNotification from the loop in an array, and resolve them all with a Promise.all outside of the loop?
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.
The idea of not puting an await here its to fire and forget the notification without asking the engine to wait for the response. So more interesting for me to add a catch function instead of await
Except my command, im ok with the concept. |
Proposed changes
Note: api-test are passing locally (with 01|02) which is not the case on current master so I think it's solving part of end of tests issues, probably proiduction issues too.
Still this PR will requires deep tests.
Related issues
Checklist
Further comments