From 365da75d1b1e613f361ac5e4cad32c965f6c46fc Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 1 Sep 2016 15:25:40 +0200 Subject: [PATCH] U4-8698 - fix notification service perfs issues --- src/Umbraco.Core/Constants-ObjectTypes.cs | 5 + .../Repositories/NotificationsRepository.cs | 7 +- .../Repositories/UserRepository.cs | 3 +- .../Services/NotificationService.cs | 182 ++++++++++-------- 4 files changed, 114 insertions(+), 83 deletions(-) diff --git a/src/Umbraco.Core/Constants-ObjectTypes.cs b/src/Umbraco.Core/Constants-ObjectTypes.cs index 7ec45db7be..3f9974166c 100644 --- a/src/Umbraco.Core/Constants-ObjectTypes.cs +++ b/src/Umbraco.Core/Constants-ObjectTypes.cs @@ -69,6 +69,11 @@ namespace Umbraco.Core /// public const string Document = "C66BA18E-EAF3-4CFF-8A22-41B16D66A972"; + /// + /// Guid for a Document object. + /// + public static readonly Guid DocumentGuid = new Guid(Document); + /// /// Guid for a Document Type object. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs index 3549f50859..9e6e3cf47c 100644 --- a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs @@ -18,15 +18,16 @@ namespace Umbraco.Core.Persistence.Repositories _unitOfWork = unitOfWork; } - public IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + public IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds, Guid objectType) { var nodeIdsA = nodeIds.ToArray(); var syntax = ApplicationContext.Current.DatabaseContext.SqlSyntax; // bah var sql = new Sql() - .Select("DISTINCT umbracoNode.id, umbracoUser2NodeNotify.userId, umbracoNode.nodeObjectType, umbracoUser2NodeNotify.action") + .Select("DISTINCT umbracoNode.id nodeId, umbracoUser.id userId, umbracoNode.nodeObjectType, umbracoUser2NodeNotify.action") .From(syntax) .InnerJoin(syntax).On(syntax, left => left.NodeId, right => right.NodeId) .InnerJoin(syntax).On(syntax, left => left.UserId, right => right.Id) + .Where(x => x.NodeObjectType == objectType) .Where(x => x.Disabled == false) // only approved users .Where(x => x.Action == action); // on the specified action if (nodeIdsA.Length > 0) @@ -35,7 +36,7 @@ namespace Umbraco.Core.Persistence.Repositories sql .OrderBy(x => x.Id, syntax) .OrderBy(dto => dto.NodeId, syntax); - return _unitOfWork.Database.Fetch(sql).Select(x => new Notification(x.id, x.userId, x.nodeObjectType, x.action)); + return _unitOfWork.Database.Fetch(sql).Select(x => new Notification(x.nodeId, x.userId, x.action, objectType)); } public IEnumerable GetUserNotifications(IUser user) diff --git a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs index 26fdb95b33..cbaa92da21 100644 --- a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs @@ -357,7 +357,8 @@ namespace Umbraco.Core.Persistence.Repositories .Where(x => x.Id >= id) .OrderBy(x => x.Id, SqlSyntax); - var ids = Database.Page(0, count, idsQuery).Items.ToArray(); + // first page is index 1, not zero + var ids = Database.Page(1, count, idsQuery).Items.ToArray(); // now get the actual users and ensure they are ordered properly (same clause) return ids.Length == 0 ? Enumerable.Empty() : GetAll(ids).OrderBy(x => x.Id); diff --git a/src/Umbraco.Core/Services/NotificationService.cs b/src/Umbraco.Core/Services/NotificationService.cs index 68b91ec82a..80509016ef 100644 --- a/src/Umbraco.Core/Services/NotificationService.cs +++ b/src/Umbraco.Core/Services/NotificationService.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -15,7 +16,6 @@ using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; using Umbraco.Core.Strings; -using umbraco.interfaces; namespace Umbraco.Core.Services { @@ -58,9 +58,9 @@ namespace Umbraco.Core.Services if (entity is IContent == false) throw new NotSupportedException(); - var content = (IContent) entity; - - // lazily get versions + var content = (IContent) entity; + + // lazily get versions List allVersions = null; // do not load *all* users in memory at once @@ -78,7 +78,7 @@ namespace Umbraco.Core.Services { // users are returned ordered by id, notifications are returned ordered by user id var users = ((UserService) _userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); - var notifications = GetUsersNotifications(users.Select(x => x.Id), action, nodeIds)/*.OrderBy(x => x.UserId)*/.ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, nodeIds, Constants.ObjectTypes.DocumentGuid).ToList(); if (notifications.Count == 0) break; var i = 0; @@ -90,16 +90,9 @@ namespace Umbraco.Core.Services // lazy load all versions if (allVersions == null) allVersions = _contentService.GetVersions(entity.Id).ToList(); - // notify - try - { - SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } + // queue notification + var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + Enqueue(req); // skip other notifications for this user while (i < notifications.Count && notifications[i++].UserId == user.Id) ; @@ -145,7 +138,7 @@ namespace Umbraco.Core.Services { // users are returned ordered by id, notifications are returned ordered by user id var users = ((UserService)_userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); - var notifications = GetUsersNotifications(users.Select(x => x.Id), action, Enumerable.Empty())/*.OrderBy(x => x.UserId)*/.ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, Enumerable.Empty(), Constants.ObjectTypes.DocumentGuid).ToList(); if (notifications.Count == 0) break; var i = 0; @@ -169,15 +162,9 @@ namespace Umbraco.Core.Services ? allVersionsDictionary[content.Id] : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); - try - { - SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } + // queue notification + var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + Enqueue(req); } // skip other notifications for this user @@ -189,44 +176,13 @@ namespace Umbraco.Core.Services id = users.Count == pagesz ? users.Last().Id + 1 : -1; } while (id > 0); - - int totalUsers; - var allUsers = _userService.GetAll(0, int.MaxValue, out totalUsers); - foreach (var u in allUsers.Where(x => x.IsApproved)) - { - var userNotifications = GetUserNotifications(u).ToArray(); - - foreach (var content in entitiesL) - { - var userNotificationsByPath = FilterUserNotificationsByPath(userNotifications, content.Path); - var notificationForAction = userNotificationsByPath.FirstOrDefault(x => x.Action == action); - if (notificationForAction == null) continue; - - var allVersions = allVersionsDictionary.ContainsKey(content.Id) - ? allVersionsDictionary[content.Id] - : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); - - try - { - SendNotification(operatingUser, u, content, allVersions, - actionName, http, createSubject, createBody); - - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", - action, u.Name, u.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } - } - } } - private IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + private IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds, Guid objectType) { var uow = _uowProvider.GetUnitOfWork(); var repository = new NotificationsRepository(uow); - return repository.GetUsersNotifications(userIds, action, nodeIds); + return repository.GetUsersNotifications(userIds, action, nodeIds, objectType); } /// @@ -340,7 +296,7 @@ namespace Umbraco.Core.Services /// /// Callback to create the mail subject /// Callback to create the mail body - private void SendNotification(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, + private NotificationRequest CreateNotificationRequest(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, Func createSubject, Func createBody) { @@ -471,26 +427,7 @@ namespace Umbraco.Core.Services string.Format("https://{0}", serverName)); } - - // send it asynchronously, we don't want to got up all of the request time to send emails! - ThreadPool.QueueUserWorkItem(state => - { - try - { - using (mail) - { - using (var sender = new SmtpClient()) - { - sender.Send(mail); - } - } - - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } - }); + return new NotificationRequest(mail, actionName, mailingUser.Name, mailingUser.Email); } private static string ReplaceLinks(string text, HttpRequestBase request) @@ -576,8 +513,95 @@ namespace Umbraco.Core.Services } // while return sb.ToString(); + } + + // manage notifications + // ideally, would need to use IBackgroundTasks - but they are not part of Core! + + private static readonly object Locker = new object(); + private static readonly BlockingCollection Queue = new BlockingCollection(); + private static volatile bool _running; + + private void Enqueue(NotificationRequest notification) + { + Queue.Add(notification); + if (_running) return; + lock (Locker) + { + if (_running) return; + Process(Queue); + _running = true; + } + } + + private class NotificationRequest + { + public NotificationRequest(MailMessage mail, string action, string userName, string email) + { + Mail = mail; + Action = action; + UserName = userName; + Email = email; + } + + public MailMessage Mail { get; private set; } + + public string Action { get; private set; } + + public string UserName { get; private set; } + + public string Email { get; private set; } } + private void Process(BlockingCollection notificationRequests) + { + ThreadPool.QueueUserWorkItem(state => + { + var s = new SmtpClient(); + try + { + _logger.Debug("Begin processing notifications."); + while (true) + { + NotificationRequest request; + while (notificationRequests.TryTake(out request, 8 * 1000)) // stay on for 8s + { + try + { + if (Sendmail != null) Sendmail(s, request.Mail, _logger); else s.Send(request.Mail); + _logger.Debug(string.Format("Notification \"{0}\" sent to {1} ({2})", request.Action, request.UserName, request.Email)); + } + catch (Exception ex) + { + _logger.Error("An error occurred sending notification", ex); + s.Dispose(); + s = new SmtpClient(); + } + finally + { + request.Mail.Dispose(); + } + } + lock (Locker) + { + if (notificationRequests.Count > 0) continue; // last chance + _running = false; // going down + break; + } + } + } + finally + { + s.Dispose(); + } + _logger.Debug("Done processing notifications."); + }); + } + + // for tests + internal static Action Sendmail; + //= (_, msg, logger) => logger.Debug("Email " + msg.To.ToString()); + #endregion } } \ No newline at end of file