From f89ce1ff26f7542ac5b2ec793f6c594126fa0533 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 28 Apr 2017 09:48:25 +0200 Subject: [PATCH 1/4] scope - cleanup --- .../Events/PassThroughEventDispatcher.cs | 4 +- ...ispatcher.cs => QueuingEventDispatcher.cs} | 10 ++- ...rBase.cs => QueuingEventDispatcherBase.cs} | 13 +++- .../UnitOfWork/IScopeUnitOfWork.cs | 1 + .../Persistence/UnitOfWork/ScopeUnitOfWork.cs | 28 +++++++- src/Umbraco.Core/Scoping/Scope.cs | 3 +- src/Umbraco.Core/Scoping/ScopeContext.cs | 64 ++++++++----------- src/Umbraco.Core/Umbraco.Core.csproj | 4 +- .../Scoping/ScopeEventDispatcherTests.cs | 2 +- src/Umbraco.Tests/Scoping/ScopeTests.cs | 40 +++++++++++- 10 files changed, 116 insertions(+), 53 deletions(-) rename src/Umbraco.Core/Events/{ScopeEventDispatcher.cs => QueuingEventDispatcher.cs} (78%) rename src/Umbraco.Core/Events/{ScopeEventDispatcherBase.cs => QueuingEventDispatcherBase.cs} (86%) diff --git a/src/Umbraco.Core/Events/PassThroughEventDispatcher.cs b/src/Umbraco.Core/Events/PassThroughEventDispatcher.cs index 1e1a9bc6d1..bc3709ea2a 100644 --- a/src/Umbraco.Core/Events/PassThroughEventDispatcher.cs +++ b/src/Umbraco.Core/Events/PassThroughEventDispatcher.cs @@ -5,8 +5,10 @@ using System.Linq; namespace Umbraco.Core.Events { /// - /// This event manager supports event cancellation and will raise the events as soon as they are tracked, it does not store tracked events + /// An IEventDispatcher that immediately raise all events. /// + /// This means that events will be raised during the scope transaction, + /// whatever happens, and the transaction could roll back in the end. internal class PassThroughEventDispatcher : IEventDispatcher { public bool DispatchCancelable(EventHandler eventHandler, object sender, CancellableEventArgs args, string eventName = null) diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs b/src/Umbraco.Core/Events/QueuingEventDispatcher.cs similarity index 78% rename from src/Umbraco.Core/Events/ScopeEventDispatcher.cs rename to src/Umbraco.Core/Events/QueuingEventDispatcher.cs index 1cf6660a40..0e10c86dde 100644 --- a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs +++ b/src/Umbraco.Core/Events/QueuingEventDispatcher.cs @@ -3,14 +3,12 @@ using Umbraco.Core.IO; namespace Umbraco.Core.Events { /// - /// This event manager is created for each scope and is aware of if it is nested in an outer scope + /// An IEventDispatcher that queues events, and raise them when the scope + /// exits and has been completed. /// - /// - /// The outer scope is the only scope that can raise events, the inner scope's will defer to the outer scope - /// - internal class ScopeEventDispatcher : ScopeEventDispatcherBase + internal class QueuingEventDispatcher : QueuingEventDispatcherBase { - public ScopeEventDispatcher() + public QueuingEventDispatcher() : base(true) { } diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs b/src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs similarity index 86% rename from src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs rename to src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs index d8462d18b3..b947835943 100644 --- a/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs +++ b/src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs @@ -4,12 +4,21 @@ using System.Linq; namespace Umbraco.Core.Events { - public abstract class ScopeEventDispatcherBase : IEventDispatcher + /// + /// An IEventDispatcher that queues events. + /// + /// + /// Can raise, or ignore, cancelable events, depending on option. + /// Implementations must override ScopeExitCompleted to define what + /// to do with the events when the scope exits and has been completed. + /// If the scope exits without being completed, events are ignored. + /// + public abstract class QueuingEventDispatcherBase : IEventDispatcher { private List _events; private readonly bool _raiseCancelable; - protected ScopeEventDispatcherBase(bool raiseCancelable) + protected QueuingEventDispatcherBase(bool raiseCancelable) { _raiseCancelable = raiseCancelable; } diff --git a/src/Umbraco.Core/Persistence/UnitOfWork/IScopeUnitOfWork.cs b/src/Umbraco.Core/Persistence/UnitOfWork/IScopeUnitOfWork.cs index e5232092c5..9c655d5d48 100644 --- a/src/Umbraco.Core/Persistence/UnitOfWork/IScopeUnitOfWork.cs +++ b/src/Umbraco.Core/Persistence/UnitOfWork/IScopeUnitOfWork.cs @@ -8,5 +8,6 @@ namespace Umbraco.Core.Persistence.UnitOfWork IScope Scope { get; } EventMessages Messages { get; } IEventDispatcher Events { get; } + void Flush(); } } \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/UnitOfWork/ScopeUnitOfWork.cs b/src/Umbraco.Core/Persistence/UnitOfWork/ScopeUnitOfWork.cs index 204e7b0bfe..f8d168cdaa 100644 --- a/src/Umbraco.Core/Persistence/UnitOfWork/ScopeUnitOfWork.cs +++ b/src/Umbraco.Core/Persistence/UnitOfWork/ScopeUnitOfWork.cs @@ -160,7 +160,33 @@ namespace Umbraco.Core.Persistence.UnitOfWork _key = Guid.NewGuid(); } - public object Key + public void Flush() + { + if (_readOnly) + throw new NotSupportedException("This unit of work is read-only."); + + while (_operations.Count > 0) + { + var operation = _operations.Dequeue(); + switch (operation.Type) + { + case TransactionType.Insert: + operation.Repository.PersistNewItem(operation.Entity); + break; + case TransactionType.Delete: + operation.Repository.PersistDeletedItem(operation.Entity); + break; + case TransactionType.Update: + operation.Repository.PersistUpdatedItem(operation.Entity); + break; + } + } + + _operations.Clear(); + _key = Guid.NewGuid(); + } + + public object Key { get { return _key; } } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index f08b9f2e6d..5d62ba2fce 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -321,7 +321,7 @@ namespace Umbraco.Core.Scoping { EnsureNotDisposed(); if (ParentScope != null) return ParentScope.Events; - return _eventDispatcher ?? (_eventDispatcher = new ScopeEventDispatcher()); + return _eventDispatcher ?? (_eventDispatcher = new QueuingEventDispatcher()); } } @@ -465,6 +465,7 @@ namespace Umbraco.Core.Scoping } finally { + // removes the ambient context (ambient scope already gone) _scopeProvider.SetAmbient(null); } } diff --git a/src/Umbraco.Core/Scoping/ScopeContext.cs b/src/Umbraco.Core/Scoping/ScopeContext.cs index cca0be560d..89697246c8 100644 --- a/src/Umbraco.Core/Scoping/ScopeContext.cs +++ b/src/Umbraco.Core/Scoping/ScopeContext.cs @@ -1,16 +1,23 @@ using System; using System.Collections.Generic; +using System.Linq; namespace Umbraco.Core.Scoping { public class ScopeContext : IInstanceIdentifiable { private Dictionary _enlisted; + private bool _exiting; public void ScopeExit(bool completed) { + if (_enlisted == null) + return; + + _exiting = true; + List exceptions = null; - foreach (var enlisted in Enlisted.Values) + foreach (var enlisted in _enlisted.Values.OrderBy(x => x.Priority)) { try { @@ -23,6 +30,7 @@ namespace Umbraco.Core.Scoping exceptions.Add(e); } } + if (exceptions != null) throw new AggregateException("Exceptions were thrown by listed actions.", exceptions); } @@ -42,73 +50,53 @@ namespace Umbraco.Core.Scoping private interface IEnlistedObject { void Execute(bool completed); + int Priority { get; } } private class EnlistedObject : IEnlistedObject { private readonly Action _action; - public EnlistedObject(T item) - { - Item = item; - } - - public EnlistedObject(T item, Action action) + public EnlistedObject(T item, Action action, int priority) { Item = item; + Priority = priority; _action = action; } public T Item { get; private set; } + public int Priority { get; private set; } + public void Execute(bool completed) { _action(completed, Item); } } - /// - public T Enlist(string key, Func creator) + public void Enlist(string key, Action action, int priority = 100) { - IEnlistedObject enlisted; - if (Enlisted.TryGetValue(key, out enlisted)) - { - var enlistedAs = enlisted as EnlistedObject; - if (enlistedAs == null) throw new Exception("An item with a different type has already been enlisted with the same key."); - return enlistedAs.Item; - } - var enlistedOfT = new EnlistedObject(creator()); - Enlisted[key] = enlistedOfT; - return enlistedOfT.Item; + Enlist(key, null, (completed, item) => action(completed), priority); } - /// - public T Enlist(string key, Func creator, Action action) + public T Enlist(string key, Func creator = null, Action action = null, int priority = 100) { + if (_exiting) + throw new InvalidOperationException("Cannot enlist now, context is exiting."); + + var enlistedObjects = _enlisted ?? (_enlisted = new Dictionary()); + IEnlistedObject enlisted; - if (Enlisted.TryGetValue(key, out enlisted)) + if (enlistedObjects.TryGetValue(key, out enlisted)) { var enlistedAs = enlisted as EnlistedObject; - if (enlistedAs == null) throw new Exception("An item with a different type has already been enlisted with the same key."); + if (enlistedAs == null) throw new InvalidOperationException("An item with the key already exists, but with a different type."); + if (enlistedAs.Priority != priority) throw new InvalidOperationException("An item with the key already exits, but with a different priority."); return enlistedAs.Item; } - var enlistedOfT = new EnlistedObject(creator(), action); + var enlistedOfT = new EnlistedObject(creator == null ? default(T) : creator(), action, priority); Enlisted[key] = enlistedOfT; return enlistedOfT.Item; } - - /// - public void Enlist(string key, Action action) - { - IEnlistedObject enlisted; - if (Enlisted.TryGetValue(key, out enlisted)) - { - var enlistedAs = enlisted as EnlistedObject; - if (enlistedAs == null) throw new Exception("An item with a different type has already been enlisted with the same key."); - return; - } - var enlistedOfT = new EnlistedObject(null, (completed, item) => action(completed)); - Enlisted[key] = enlistedOfT; - } } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 74cbe5d534..b113914fc0 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -337,7 +337,7 @@ - + @@ -377,7 +377,7 @@ - + diff --git a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs index b68e2a3f9f..01df994e0e 100644 --- a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs @@ -183,7 +183,7 @@ namespace Umbraco.Tests.Scoping public static event TypedEventHandler> DoThing3; - public class PassiveEventDispatcher : ScopeEventDispatcherBase + public class PassiveEventDispatcher : QueuingEventDispatcherBase { public PassiveEventDispatcher() : base(false) diff --git a/src/Umbraco.Tests/Scoping/ScopeTests.cs b/src/Umbraco.Tests/Scoping/ScopeTests.cs index 747e5b1f40..6e24bcf51c 100644 --- a/src/Umbraco.Tests/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeTests.cs @@ -591,7 +591,7 @@ namespace Umbraco.Tests.Scoping Assert.IsNull(scopeProvider.AmbientContext); // back to original thread - ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; } Assert.IsNotNull(scopeProvider.AmbientScope); Assert.AreSame(scope, scopeProvider.AmbientScope); @@ -656,6 +656,44 @@ namespace Umbraco.Tests.Scoping Assert.IsNotNull(ambientContext); // the context is still there } + [TestCase(true)] + [TestCase(false)] + public void ScopeContextEnlistAgain(bool complete) + { + var scopeProvider = DatabaseContext.ScopeProvider; + + bool? completed = null; + Exception exception = null; + + Assert.IsNull(scopeProvider.AmbientScope); + using (var scope = scopeProvider.CreateScope()) + { + scopeProvider.Context.Enlist("name", c => + { + completed = c; + + // at that point the scope is gone, but the context is still there + var ambientContext = scopeProvider.AmbientContext; + + try + { + ambientContext.Enlist("another", c2 => { }); + } + catch (Exception e) + { + exception = e; + } + }); + if (complete) + scope.Complete(); + } + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + Assert.IsNotNull(completed); + Assert.IsNotNull(exception); + Assert.IsInstanceOf(exception); + } + [Test] public void ScopeContextException() { From 8c6aa9875d417d959c4507691feed7ece8bdd74a Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 28 Apr 2017 11:26:41 +0200 Subject: [PATCH 2/4] scope - dont rename --- .../{QueuingEventDispatcher.cs => ScopeEventDispatcher.cs} | 4 ++-- ...uingEventDispatcherBase.cs => ScopeEventDispatcherBase.cs} | 4 ++-- src/Umbraco.Core/Scoping/Scope.cs | 2 +- src/Umbraco.Core/Umbraco.Core.csproj | 4 ++-- src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) rename src/Umbraco.Core/Events/{QueuingEventDispatcher.cs => ScopeEventDispatcher.cs} (92%) rename src/Umbraco.Core/Events/{QueuingEventDispatcherBase.cs => ScopeEventDispatcherBase.cs} (96%) diff --git a/src/Umbraco.Core/Events/QueuingEventDispatcher.cs b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs similarity index 92% rename from src/Umbraco.Core/Events/QueuingEventDispatcher.cs rename to src/Umbraco.Core/Events/ScopeEventDispatcher.cs index 0e10c86dde..eb4f8ec34e 100644 --- a/src/Umbraco.Core/Events/QueuingEventDispatcher.cs +++ b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs @@ -6,9 +6,9 @@ namespace Umbraco.Core.Events /// An IEventDispatcher that queues events, and raise them when the scope /// exits and has been completed. /// - internal class QueuingEventDispatcher : QueuingEventDispatcherBase + internal class ScopeEventDispatcher : ScopeEventDispatcherBase { - public QueuingEventDispatcher() + public ScopeEventDispatcher() : base(true) { } diff --git a/src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs similarity index 96% rename from src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs rename to src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs index b947835943..4dcb15515a 100644 --- a/src/Umbraco.Core/Events/QueuingEventDispatcherBase.cs +++ b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs @@ -13,12 +13,12 @@ namespace Umbraco.Core.Events /// to do with the events when the scope exits and has been completed. /// If the scope exits without being completed, events are ignored. /// - public abstract class QueuingEventDispatcherBase : IEventDispatcher + public abstract class ScopeEventDispatcherBase : IEventDispatcher { private List _events; private readonly bool _raiseCancelable; - protected QueuingEventDispatcherBase(bool raiseCancelable) + protected ScopeEventDispatcherBase(bool raiseCancelable) { _raiseCancelable = raiseCancelable; } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 5d62ba2fce..bfee772872 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -321,7 +321,7 @@ namespace Umbraco.Core.Scoping { EnsureNotDisposed(); if (ParentScope != null) return ParentScope.Events; - return _eventDispatcher ?? (_eventDispatcher = new QueuingEventDispatcher()); + return _eventDispatcher ?? (_eventDispatcher = new ScopeEventDispatcher()); } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index b113914fc0..74cbe5d534 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -337,7 +337,7 @@ - + @@ -377,7 +377,7 @@ - + diff --git a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs index 01df994e0e..b68e2a3f9f 100644 --- a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs @@ -183,7 +183,7 @@ namespace Umbraco.Tests.Scoping public static event TypedEventHandler> DoThing3; - public class PassiveEventDispatcher : QueuingEventDispatcherBase + public class PassiveEventDispatcher : ScopeEventDispatcherBase { public PassiveEventDispatcher() : base(false) From efc3261163ca663299bc88c69ac36a559490e3da Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 28 Apr 2017 11:43:48 +0200 Subject: [PATCH 3/4] scope - dont break --- src/Umbraco.Core/Scoping/ScopeContext.cs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Scoping/ScopeContext.cs b/src/Umbraco.Core/Scoping/ScopeContext.cs index 89697246c8..c79f02a0fa 100644 --- a/src/Umbraco.Core/Scoping/ScopeContext.cs +++ b/src/Umbraco.Core/Scoping/ScopeContext.cs @@ -74,12 +74,30 @@ namespace Umbraco.Core.Scoping } } - public void Enlist(string key, Action action, int priority = 100) + // todo: replace with optional parameters when we can break things + public T Enlist(string key, Func creator) + { + return Enlist(key, creator, null, 100); + } + + // todo: replace with optional parameters when we can break things + public T Enlist(string key, Func creator, Action action) + { + return Enlist(key, creator, action, 100); + } + + // todo: replace with optional parameters when we can break things + public void Enlist(string key, Action action) + { + Enlist(key, null, (completed, item) => action(completed), 100); + } + + public void Enlist(string key, Action action, int priority) { Enlist(key, null, (completed, item) => action(completed), priority); } - public T Enlist(string key, Func creator = null, Action action = null, int priority = 100) + public T Enlist(string key, Func creator, Action action, int priority) { if (_exiting) throw new InvalidOperationException("Cannot enlist now, context is exiting."); From 244977dc95d47668e0309bb8f55fead3b7426cbb Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 28 Apr 2017 13:34:35 +0200 Subject: [PATCH 4/4] packages - trace errors --- .../Editors/PackageInstallController.cs | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Web/Editors/PackageInstallController.cs b/src/Umbraco.Web/Editors/PackageInstallController.cs index c980a06521..d3771ce82c 100644 --- a/src/Umbraco.Web/Editors/PackageInstallController.cs +++ b/src/Umbraco.Web/Editors/PackageInstallController.cs @@ -58,17 +58,25 @@ namespace Umbraco.Web.Editors [HttpPost] public IHttpActionResult Uninstall(int packageId) { - var pack = InstalledPackage.GetById(packageId); - if (pack == null) return NotFound(); - - PerformUninstall(pack); - - //now get all other packages by this name since we'll uninstall all versions - foreach (var installed in InstalledPackage.GetAllInstalledPackages() - .Where(x => x.Data.Name == pack.Data.Name && x.Data.Id != pack.Data.Id)) + try { - //remove from teh xml - installed.Delete(Security.GetUserId()); + var pack = InstalledPackage.GetById(packageId); + if (pack == null) return NotFound(); + + PerformUninstall(pack); + + //now get all other packages by this name since we'll uninstall all versions + foreach (var installed in InstalledPackage.GetAllInstalledPackages() + .Where(x => x.Data.Name == pack.Data.Name && x.Data.Id != pack.Data.Id)) + { + //remove from teh xml + installed.Delete(Security.GetUserId()); + } + } + catch (Exception e) + { + Logger.Error("Failed to uninstall.", e); + throw; } return Ok(); @@ -176,7 +184,7 @@ namespace Umbraco.Web.Editors pack.Save(); // uninstall actions - //TODO: We should probably report errors to the UI!! + //TODO: We should probably report errors to the UI!! // This never happened before though, but we should do something now if (pack.Data.Actions.IsNullOrWhiteSpace() == false) { @@ -471,7 +479,7 @@ namespace Umbraco.Web.Editors string path = Path.Combine("packages", packageGuid + ".umb"); if (File.Exists(IOHelper.MapPath(Path.Combine(SystemDirectories.Data, path))) == false) { - path = Services.PackagingService.FetchPackageFile(Guid.Parse(packageGuid), UmbracoVersion.Current, Security.GetUserId()); + path = Services.PackagingService.FetchPackageFile(Guid.Parse(packageGuid), UmbracoVersion.Current, Security.GetUserId()); } var model = new LocalPackageInstallModel