[bug] too many unread notifications cause system memory overflow

sleede · Jan 5, 2017 · c479502 · c479502
1 parent 7f98cdb
commit c479502
Show file tree

Hide file tree

Showing 14 changed files with 164 additions and 85 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -19,7 +19,7 @@
   <div class="row">
 
     <div class="col-md-12">
-      <button type="button" class="btn btn-warning m-t-sm m-b" ng-click="markAllAsRead()" ng-disabled="notifications.length == 0">{{ 'mark_all_as_read' | translate }} ({{notifications.length}})</button>
+      <button type="button" class="btn btn-warning m-t-sm m-b" ng-click="markAllAsRead()" ng-disabled="totalUnread == 0">{{ 'mark_all_as_read' | translate }} ({{totalUnread}})</button>
 
       <table class="table">
         <thead>
@@ -31,7 +31,7 @@
           </tr>
         </thead>
         <tbody>
-          <tr ng-repeat="notification in notifications" ng-if="notifications.length > 0">
+          <tr ng-repeat="notification in notificationsUnread" ng-if="notificationsUnread.length > 0">
             <td>
                 <button class="btn btn-sm btn-warning" ng-click="markAsRead(notification, $event)">
                   <i class="fa fa-check"></i>
@@ -41,45 +41,47 @@
             <td ng-bind-html="notification.message.description"></td>
 
           </tr>
-          <tr ng-if="notifications.length == 0">
+          <tr ng-if="notificationsUnread.length == 0">
             <td colspan="3" translate>{{ 'no_new_notifications' }}</td>
           </tr>
         </tbody>
       </table>
 
-      <h5 translate>{{ 'archives' }}</h5>
+      <div ng-hide="notificationsRead.length == 0 && notificationsUnread.length < total">
+        <h5 translate>{{ 'archives' }}</h5>
 
-      <table class="table">
-        <thead>
-          <tr>
-            <th style="width:10%"></th>
-            <th style="width:20%"></th>
-            <th style="width:70%"></th>
+        <table class="table">
+          <thead>
+            <tr>
+              <th style="width:10%"></th>
+              <th style="width:20%"></th>
+              <th style="width:70%"></th>
 
-          </tr>
-        </thead>
-        <tbody>
+            </tr>
+          </thead>
+          <tbody>
 
 
 
-          <tr class="read" ng-repeat="n in notificationsRead | orderBy:'created_at':true" ng-if="notificationsRead.length > 0">
-            <td>
-            </td>
-            <td>{{ n.created_at | amDateFormat:'LLL' }}</td>
-            <td ng-bind-html="n.message.description"></td>
+            <tr class="read" ng-repeat="n in notificationsRead | orderBy:'created_at':true" ng-if="notificationsRead.length > 0">
+              <td>
+              </td>
+              <td>{{ n.created_at | amDateFormat:'LLL' }}</td>
+              <td ng-bind-html="n.message.description"></td>
 
-          </tr>
+            </tr>
 
 
-          <tr ng-if="notificationsRead.length == 0">
-            <td colspan="3" translate>{{ 'no_archived_notifications' }}</td>
-          </tr>
+            <tr ng-if="notificationsRead.length == 0">
+              <td colspan="3" translate>{{ 'no_archived_notifications' }}</td>
+            </tr>
 
 
-        </tbody>
-      </table>
+          </tbody>
+        </table>
+      </div>
 
-      <a class="btn btn-default" ng-click="addMoreNotificationsReaded()" ng-if="paginateActive" translate>{{ 'load_the_next_notifications' }}</a>
+      <a class="btn btn-default" ng-click="addMoreNotifications()" ng-if="paginateActive" translate>{{ 'load_the_next_notifications' }}</a>
 
     </div>
 

diff --git a/app/assets/templates/shared/header.html.erb b/app/assets/templates/shared/header.html.erb
@@ -23,7 +23,7 @@
   <!-- Top Nav -->
   <ul class="nav navbar-nav navbar-right m-n hidden-xs nav-user user">
     <li class="notification-open" ng-if="isAuthenticated()">
-      <a ui-sref="app.logged.notifications"><i class="fa fa-bell fa-2x black"></i> <span class="badge" ng-class="{'bg-red': notifications.length > 0}">{{notifications.length}}</span></a>
+      <a ui-sref="app.logged.notifications"><i class="fa fa-bell fa-2x black"></i> <span class="badge" ng-class="{'bg-red': notifications.unread > 0}">{{notifications.unread}}</span></a>
     </li>
     <li class="dropdown user-profile-nav" ng-if="isAuthenticated()" uib-dropdown>
       <a href="#" class="dropdown-toggle" uib-dropdown-toggle>

diff --git a/app/controllers/api/notifications_controller.rb b/app/controllers/api/notifications_controller.rb
@@ -3,14 +3,49 @@ class API::NotificationsController < API::ApiController
   before_action :authenticate_user!
 
   def index
-    if params[:is_read]
-      if params[:is_read] == 'true'
-        @notifications = current_user.notifications.where(is_read: true).page(params[:page]).per(15).order('created_at DESC')
-      else
-        @notifications = current_user.notifications.where(is_read: false).order('created_at DESC')
+    loop do
+      @notifications = current_user.notifications.page(params[:page]).per(15).order('created_at DESC')
+      # we delete obsolete notifications on first access
+      break unless delete_obsoletes(@notifications)
+    end
+    @totals = {
+        total: current_user.notifications.count,
+        unread: current_user.notifications.where(is_read: false).count
+    }
+    render :index
+  end
+
+  def last_unread
+    loop do
+      @notifications = current_user.notifications.where(is_read: false).limit(3).order('created_at DESC')
+      # we delete obsolete notifications on first access
+      break unless delete_obsoletes(@notifications)
+    end
+    @totals = {
+        total: current_user.notifications.count,
+        unread: current_user.notifications.where(is_read: false).count
+    }
+    render :index
+  end
+
+  def polling
+    @notifications = current_user.notifications.where('is_read = false AND created_at >= :date', date: params[:last_poll]).order('created_at DESC')
+    @totals = {
+        total: current_user.notifications.count,
+        unread: current_user.notifications.where(is_read: false).count
+    }
+    render :index
+  end
+
+  private
+  def delete_obsoletes(notifications)
+    cleaned = false
+    notifications.each do |n|
+      if !Module.const_get(n.attached_object_type) or !n.attached_object
+        n.destroy!
+        cleaned = true
       end
-    else
-      @notifications = current_user.notifications.order('created_at DESC')
     end
+    cleaned
   end
 end
diff --git a/app/views/api/notifications/index.json.jbuilder b/app/views/api/notifications/index.json.jbuilder
@@ -1,13 +1,12 @@
-json.array!(@notifications) do |notification|
-  if Module.const_get(notification.attached_object_type) and notification.attached_object # WHY WERE WE DOING Object.const_defined?(notification.attached_object_type) ??? Why not just deleting obsolete notifications ?! Object.const_defined? was introducing a bug! Module.const_get is a TEMPORARY fix, NOT a solution
-    json.extract! notification, :id, :notification_type_id, :notification_type, :created_at, :is_read
-    json.attached_object notification.attached_object
-    json.message do
-      if notification.notification_type.nil?
-        json.partial! 'api/notifications/undefined_notification', notification: notification
-      else
-        json.partial! "/api/notifications/#{notification.notification_type}", notification: notification
-      end
+json.totals @totals
+json.notifications(@notifications) do |notification|
+  json.extract! notification, :id, :notification_type_id, :notification_type, :created_at, :is_read
+  json.attached_object notification.attached_object
+  json.message do
+    if notification.notification_type.nil?
+      json.partial! 'api/notifications/undefined_notification', notification: notification
+    else
+      json.partial! "/api/notifications/#{notification.notification_type}", notification: notification
     end
   end
 end.delete_if {|n| n['id'] == nil }
diff --git a/config/locales/app.public.en.yml b/config/locales/app.public.en.yml
@@ -104,7 +104,7 @@ en:
         version: "Version:"
 
         # Notifications
-        and_NUMBER_other_notifications: "and {{NUMBER}} other notifications..."  # angular interpolation
+        and_NUMBER_other_notifications: "and {NUMBER, plural, =0{no other notifications} =1{one other notification} other{{NUMBER} other notifications}}..."  # messageFormat interpolation
 
       about:
         # about page