From c479502dd0ff63fbc000c0bff6ea97c6f9baedaf Mon Sep 17 00:00:00 2001 From: Sylvain Date: Thu, 5 Jan 2017 15:06:54 +0100 Subject: [PATCH 1/6] [bug] too many unread notifications cause system memory overflow --- CHANGELOG.md | 5 ++ .../controllers/application.coffee.erb | 43 +++++++-------- .../javascripts/controllers/members.coffee | 4 +- .../controllers/notifications.coffee | 55 ++++++++++++++----- .../controllers/profile.coffee.erb | 4 +- .../javascripts/services/notification.coffee | 8 +++ .../templates/notifications/index.html.erb | 52 +++++++++--------- app/assets/templates/shared/header.html.erb | 2 +- .../api/notifications_controller.rb | 49 ++++++++++++++--- .../api/notifications/index.json.jbuilder | 19 +++---- config/locales/app.public.en.yml | 2 +- config/locales/app.public.fr.yml | 2 +- config/locales/fr.yml | 2 +- config/routes.rb | 2 + 14 files changed, 164 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8cbe8c549..ae5f1f8f86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog Fab Manager +## v2.4.10 2017 January 5 + +- Optimized notifications system +- Fix a bug: when many users with too many unread notifications are connected at the same time, the system kill the application due to memory overflow + ## v2.4.9 2017 January 4 - Mask new notifications alerts when more than 3 diff --git a/app/assets/javascripts/controllers/application.coffee.erb b/app/assets/javascripts/controllers/application.coffee.erb index 645b6e6cb9..683c2e3ffb 100644 --- a/app/assets/javascripts/controllers/application.coffee.erb +++ b/app/assets/javascripts/controllers/application.coffee.erb @@ -60,7 +60,9 @@ Application.Controllers.controller 'ApplicationController', ["$rootScope", "$sco Session.destroy() $rootScope.currentUser = null $rootScope.toCheckNotifications = false - $scope.notifications = [] + $scope.notifications = + total: 0 + unread: 0 $state.go('app.public.home') , (error) -> # An error occurred logging out. @@ -261,33 +263,30 @@ Application.Controllers.controller 'ApplicationController', ["$rootScope", "$sco $rootScope.toCheckNotifications = true unless $rootScope.checkNotificationsIsInit or !$rootScope.currentUser setTimeout -> - $scope.notifications = Notification.query {is_read: false} - , 2000 - $scope.$watch 'notifications', (newValue, oldValue) -> - diff = [] - angular.forEach newValue, (value) -> - find = false - for i in [0..oldValue.length] by 1 - if oldValue[i] and (value.id is oldValue[i].id) - find = true - break - - unless find - diff.push(value) + # we request the most recent notifications + Notification.last_unread (notifications) -> + $rootScope.lastCheck = new Date() + $scope.notifications = notifications.totals - remain = 3 - if diff.length >= remain - diff.splice(remain, (diff.length - remain), {message: {description: _t('and_NUMBER_other_notifications', {NUMBER: diff.length - remain})}}) + toDisplay = [] + angular.forEach notifications.notifications, (n) -> + toDisplay.push(n) - angular.forEach diff, (notification, key) -> - growl.info(notification.message.description) + if toDisplay.length < notifications.totals.unread + toDisplay.push({message: {description: _t('and_NUMBER_other_notifications', {NUMBER: notifications.totals.unread - toDisplay.length}, "messageformat")}}) - , true + angular.forEach toDisplay, (notification) -> + growl.info(notification.message.description) + , 2000 checkNotifications = -> if $rootScope.toCheckNotifications - Notification.query({is_read: false}).$promise.then (data) -> - $scope.notifications = data; + Notification.polling({last_poll: $rootScope.lastCheck}).$promise.then (data) -> + $rootScope.lastCheck = new Date() + $scope.notifications = data.totals + + angular.forEach data.notifications, (notification) -> + growl.info(notification.message.description) $interval(checkNotifications, NOTIFICATIONS_CHECK_PERIOD) $rootScope.checkNotificationsIsInit = true diff --git a/app/assets/javascripts/controllers/members.coffee b/app/assets/javascripts/controllers/members.coffee index f71b8292c8..3c2c96e56e 100644 --- a/app/assets/javascripts/controllers/members.coffee +++ b/app/assets/javascripts/controllers/members.coffee @@ -225,7 +225,9 @@ Application.Controllers.controller "EditProfileController", ["$scope", "$rootSco Session.destroy() $rootScope.currentUser = null $rootScope.toCheckNotifications = false - $scope.notifications = [] + $scope.notifications = + total: 0 + unread: 0 $window.location.href = $scope.activeProvider.link_to_sso_connect diff --git a/app/assets/javascripts/controllers/notifications.coffee b/app/assets/javascripts/controllers/notifications.coffee index 3426aa1d08..9c2e3ade28 100644 --- a/app/assets/javascripts/controllers/notifications.coffee +++ b/app/assets/javascripts/controllers/notifications.coffee @@ -2,7 +2,7 @@ ## # Controller used in notifications page -# inherits $scope.$parent.notifications (unread notifications) from ApplicationController +# inherits $scope.$parent.notifications (global notifications state) from ApplicationController ## Application.Controllers.controller "NotificationsController", ["$scope", 'Notification', ($scope, Notification) -> @@ -20,6 +20,15 @@ Application.Controllers.controller "NotificationsController", ["$scope", 'Notifi ## Array containg the archived notifications (already read) $scope.notificationsRead = [] + ## Array containg the new notifications (not read) + $scope.notificationsUnread = [] + + ## Total number of notifications for the current user + $scope.total = 0 + + ## Total number of unread notifications for the current user + $scope.totalUnread = 0 + ## By default, the pagination mode is activated to limit the page size $scope.paginateActive = true @@ -39,10 +48,15 @@ Application.Controllers.controller "NotificationsController", ["$scope", 'Notifi Notification.update {id: notification.id}, id: notification.id is_read: true - , -> - index = $scope.$parent.notifications.indexOf(notification) - $scope.$parent.notifications.splice(index,1) - $scope.notificationsRead.push notification + , (updatedNotif) -> + # remove notif from unreads + index = $scope.notificationsUnread.indexOf(notification) + $scope.notificationsUnread.splice(index,1) + # add update notif to read + $scope.notificationsRead.push updatedNotif + # update counters + $scope.$parent.notifications.unread -= 1 + $scope.totalUnread -= 1 @@ -52,21 +66,32 @@ Application.Controllers.controller "NotificationsController", ["$scope", 'Notifi $scope.markAllAsRead = -> Notification.update {} , -> # success - angular.forEach $scope.$parent.notifications, (n)-> + # add notifs to read + angular.forEach $scope.notificationsUnread, (n)-> + n.is_read = true $scope.notificationsRead.push n - - $scope.$parent.notifications.splice(0, $scope.$parent.notifications.length) + # clear unread + $scope.notificationsUnread = [] + # update counters + $scope.$parent.notifications.unread = 0 + $scope.totalUnread = 0 ## - # Request the server to retrieve the next undisplayed notifications and add them - # to the archived notifications list. + # Request the server to retrieve the next notifications and add them + # to their corresponding notifications list (read or unread). ## - $scope.addMoreNotificationsReaded = -> - Notification.query {is_read: true, page: $scope.page}, (notifications) -> - $scope.notificationsRead = $scope.notificationsRead.concat notifications - $scope.paginateActive = false if notifications.length < NOTIFICATIONS_PER_PAGE + $scope.addMoreNotifications = -> + Notification.query {page: $scope.page}, (notifications) -> + $scope.total = notifications.totals.total + $scope.totalUnread = notifications.totals.unread + angular.forEach notifications.notifications, (notif) -> + if notif.is_read + $scope.notificationsRead.push(notif) + else + $scope.notificationsUnread.push(notif) + $scope.paginateActive = (notifications.totals.total > ($scope.notificationsRead.length + $scope.notificationsUnread.length)) $scope.page += 1 @@ -78,7 +103,7 @@ Application.Controllers.controller "NotificationsController", ["$scope", 'Notifi # Kind of constructor: these actions will be realized first when the controller is loaded ## initialize = -> - $scope.addMoreNotificationsReaded() + $scope.addMoreNotifications() diff --git a/app/assets/javascripts/controllers/profile.coffee.erb b/app/assets/javascripts/controllers/profile.coffee.erb index 781f12995f..c5ca54280c 100644 --- a/app/assets/javascripts/controllers/profile.coffee.erb +++ b/app/assets/javascripts/controllers/profile.coffee.erb @@ -170,7 +170,9 @@ Application.Controllers.controller "CompleteProfileController", ["$scope", "$roo Session.destroy() $rootScope.currentUser = null $rootScope.toCheckNotifications = false - $scope.notifications = [] + $scope.notifications = + total: 0 + unread: 0 $window.location.href = activeProviderPromise.link_to_sso_connect diff --git a/app/assets/javascripts/services/notification.coffee b/app/assets/javascripts/services/notification.coffee index 56709c43cd..42eb9e9e29 100644 --- a/app/assets/javascripts/services/notification.coffee +++ b/app/assets/javascripts/services/notification.coffee @@ -3,6 +3,14 @@ Application.Services.factory 'Notification', ["$resource", ($resource)-> $resource "/api/notifications/:id", {id: "@id"}, + query: + isArray: false update: method: 'PUT' + polling: + url: '/api/notifications/polling' + method: 'GET' + last_unread: + url: '/api/notifications/last_unread' + method: 'GET' ] diff --git a/app/assets/templates/notifications/index.html.erb b/app/assets/templates/notifications/index.html.erb index e4d3f00032..1d244d4cdb 100644 --- a/app/assets/templates/notifications/index.html.erb +++ b/app/assets/templates/notifications/index.html.erb @@ -19,7 +19,7 @@
- + @@ -31,7 +31,7 @@ - + - +
{{ 'no_new_notifications' }}
-
{{ 'archives' }}
+
+
{{ 'archives' }}
- - - - - - +
+ + + + + - - - + + + - - - - + + + + - + - - - + + + - -
- {{ n.created_at | amDateFormat:'LLL' }}
+ {{ n.created_at | amDateFormat:'LLL' }}
{{ 'no_archived_notifications' }}
{{ 'no_archived_notifications' }}
+ + +
- {{ 'load_the_next_notifications' }} + {{ 'load_the_next_notifications' }}
diff --git a/app/assets/templates/shared/header.html.erb b/app/assets/templates/shared/header.html.erb index ed5bfef099..3a2c59502c 100644 --- a/app/assets/templates/shared/header.html.erb +++ b/app/assets/templates/shared/header.html.erb @@ -23,7 +23,7 @@