From 308a61895e085403fb91da5c1e89fc5431f1e19e Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 9 Feb 2017 14:53:03 +0100 Subject: [PATCH 01/12] deploy-219 - implement filesystem CanAddPhysical, AddFile from physical path --- src/Umbraco.Core/IO/FileSystemWrapper.cs | 19 ++++++++ src/Umbraco.Core/IO/IFileSystem.cs | 4 ++ src/Umbraco.Core/IO/PhysicalFileSystem.cs | 23 ++++++++++ src/Umbraco.Core/IO/ShadowFileSystem.cs | 43 ++++++++++++++++++- src/Umbraco.Core/IO/ShadowWrapper.cs | 17 ++++++++ src/Umbraco.Tests/IO/ShadowFileSystemTests.cs | 5 ++- 6 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/IO/FileSystemWrapper.cs b/src/Umbraco.Core/IO/FileSystemWrapper.cs index 27e08330ed..97b8a2f8f6 100644 --- a/src/Umbraco.Core/IO/FileSystemWrapper.cs +++ b/src/Umbraco.Core/IO/FileSystemWrapper.cs @@ -109,5 +109,24 @@ namespace Umbraco.Core.IO var wrapped2 = Wrapped as IFileSystem2; return wrapped2 == null ? Wrapped.GetSize(path) : wrapped2.GetSize(path); } + + // explicitely implementing - not breaking + bool IFileSystem2.CanAddPhysical + { + get + { + var wrapped2 = Wrapped as IFileSystem2; + return wrapped2 != null && wrapped2.CanAddPhysical; + } + } + + // explicitely implementing - not breaking + void IFileSystem2.AddFile(string path, string physicalPath, bool overrideIfExists, bool copy) + { + var wrapped2 = Wrapped as IFileSystem2; + if (wrapped2 == null) + throw new NotSupportedException(); + wrapped2.AddFile(path, physicalPath, overrideIfExists, copy); + } } } \ No newline at end of file diff --git a/src/Umbraco.Core/IO/IFileSystem.cs b/src/Umbraco.Core/IO/IFileSystem.cs index 003b4891f5..e3e0f9e2d2 100644 --- a/src/Umbraco.Core/IO/IFileSystem.cs +++ b/src/Umbraco.Core/IO/IFileSystem.cs @@ -44,6 +44,10 @@ namespace Umbraco.Core.IO { long GetSize(string path); + bool CanAddPhysical { get; } + + void AddFile(string path, string physicalPath, bool overrideIfExists = true, bool copy = false); + // TODO: implement these // //void CreateDirectory(string path); diff --git a/src/Umbraco.Core/IO/PhysicalFileSystem.cs b/src/Umbraco.Core/IO/PhysicalFileSystem.cs index 33e4dc71e3..bf18d02e54 100644 --- a/src/Umbraco.Core/IO/PhysicalFileSystem.cs +++ b/src/Umbraco.Core/IO/PhysicalFileSystem.cs @@ -364,6 +364,29 @@ namespace Umbraco.Core.IO return file.Exists ? file.Length : -1; } + public bool CanAddPhysical { get { return true; } } + + public void AddFile(string path, string physicalPath, bool overrideIfExists = true, bool copy = false) + { + var fullPath = GetFullPath(path); + + if (File.Exists(fullPath)) + { + if (overrideIfExists == false) + throw new InvalidOperationException(string.Format("A file at path '{0}' already exists", path)); + File.Delete(fullPath); + } + + var directory = Path.GetDirectoryName(fullPath); + if (directory == null) throw new InvalidOperationException("Could not get directory."); + Directory.CreateDirectory(directory); // ensure it exists + + if (copy) + File.Copy(physicalPath, fullPath); + else + File.Move(physicalPath, fullPath); + } + #region Helper Methods protected virtual void EnsureDirectory(string path) diff --git a/src/Umbraco.Core/IO/ShadowFileSystem.cs b/src/Umbraco.Core/IO/ShadowFileSystem.cs index 1e5da10bdc..be7c90acea 100644 --- a/src/Umbraco.Core/IO/ShadowFileSystem.cs +++ b/src/Umbraco.Core/IO/ShadowFileSystem.cs @@ -33,8 +33,16 @@ namespace Umbraco.Core.IO { try { - using (var stream = _sfs.OpenFile(kvp.Key)) - _fs.AddFile(kvp.Key, stream, true); + var fs2 = _fs as IFileSystem2; + if (fs2 != null && fs2.CanAddPhysical) + { + fs2.AddFile(kvp.Key, _sfs.GetFullPath(kvp.Key)); // overwrite, move + } + else + { + using (var stream = _sfs.OpenFile(kvp.Key)) + _fs.AddFile(kvp.Key, stream, true); + } } catch (Exception e) { @@ -282,5 +290,36 @@ namespace Umbraco.Core.IO if (sf.IsDelete || sf.IsDir) throw new InvalidOperationException("Invalid path."); return _sfs.GetSize(path); } + + public bool CanAddPhysical { get { return true; } } + + public void AddFile(string path, string physicalPath, bool overrideIfExists = true, bool copy = false) + { + ShadowNode sf; + var normPath = NormPath(path); + if (Nodes.TryGetValue(normPath, out sf) && sf.IsExist && (sf.IsDir || overrideIfExists == false)) + throw new InvalidOperationException(string.Format("A file at path '{0}' already exists", path)); + + var parts = normPath.Split('/'); + for (var i = 0; i < parts.Length - 1; i++) + { + var dirPath = string.Join("/", parts.Take(i + 1)); + ShadowNode sd; + if (Nodes.TryGetValue(dirPath, out sd)) + { + if (sd.IsFile) throw new InvalidOperationException("Invalid path."); + if (sd.IsDelete) Nodes[dirPath] = new ShadowNode(false, true); + } + else + { + if (_fs.DirectoryExists(dirPath)) continue; + if (_fs.FileExists(dirPath)) throw new InvalidOperationException("Invalid path."); + Nodes[dirPath] = new ShadowNode(false, true); + } + } + + _sfs.AddFile(path, physicalPath, overrideIfExists, copy); + Nodes[normPath] = new ShadowNode(false, false); + } } } diff --git a/src/Umbraco.Core/IO/ShadowWrapper.cs b/src/Umbraco.Core/IO/ShadowWrapper.cs index 7c8bd55830..503791226f 100644 --- a/src/Umbraco.Core/IO/ShadowWrapper.cs +++ b/src/Umbraco.Core/IO/ShadowWrapper.cs @@ -164,5 +164,22 @@ namespace Umbraco.Core.IO var filesystem2 = filesystem as IFileSystem2; return filesystem2 == null ? filesystem.GetSize(path) : filesystem2.GetSize(path); } + + public bool CanAddPhysical + { + get + { + var fileSystem2 = FileSystem as IFileSystem2; + return fileSystem2 != null && fileSystem2.CanAddPhysical; + } + } + + public void AddFile(string path, string physicalPath, bool overrideIfExists = true, bool copy = false) + { + var fileSystem2 = FileSystem as IFileSystem2; + if (fileSystem2 == null) + throw new NotSupportedException(); + fileSystem2.AddFile(path, physicalPath, overrideIfExists, copy); + } } } \ No newline at end of file diff --git a/src/Umbraco.Tests/IO/ShadowFileSystemTests.cs b/src/Umbraco.Tests/IO/ShadowFileSystemTests.cs index 8e79544052..971ef25046 100644 --- a/src/Umbraco.Tests/IO/ShadowFileSystemTests.cs +++ b/src/Umbraco.Tests/IO/ShadowFileSystemTests.cs @@ -364,7 +364,8 @@ namespace Umbraco.Tests.IO ss.Complete(); - Assert.IsTrue(File.Exists(path + "/ShadowSystem/path/to/some/dir/f1.txt")); // *not* cleaning + // yes we are cleaning now + //Assert.IsTrue(File.Exists(path + "/ShadowSystem/path/to/some/dir/f1.txt")); // *not* cleaning Assert.IsTrue(File.Exists(path + "/ShadowTests/path/to/some/dir/f1.txt")); Assert.IsFalse(File.Exists(path + "/ShadowTests/sub/sub/f2.txt")); } @@ -558,7 +559,7 @@ namespace Umbraco.Tests.IO Assert.AreEqual(1, ae.InnerExceptions.Count); e = ae.InnerExceptions[0]; Assert.IsNotNull(e.InnerException); - Assert.IsInstanceOf(e.InnerException); + Assert.IsInstanceOf(e.InnerException); } // still, the rest of the changes has been applied ok From 82c18955b0564e3321169887e083cba89005e305 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 10 Feb 2017 14:38:50 +0100 Subject: [PATCH 02/12] Version 7.6-alpha060 --- build/UmbracoVersion.txt | 2 +- src/SolutionInfo.cs | 2 +- src/Umbraco.Core/Configuration/UmbracoVersion.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build/UmbracoVersion.txt b/build/UmbracoVersion.txt index 0294012380..cb641006d1 100644 --- a/build/UmbracoVersion.txt +++ b/build/UmbracoVersion.txt @@ -1,3 +1,3 @@ # Usage: on line 2 put the release version, on line 3 put the version comment (example: beta) 7.6.0 -alpha056 \ No newline at end of file +alpha060 diff --git a/src/SolutionInfo.cs b/src/SolutionInfo.cs index 7fc240f4f0..8288c293e4 100644 --- a/src/SolutionInfo.cs +++ b/src/SolutionInfo.cs @@ -12,4 +12,4 @@ using System.Resources; [assembly: AssemblyVersion("1.0.*")] [assembly: AssemblyFileVersion("7.6.0")] -[assembly: AssemblyInformationalVersion("7.6.0-alpha056")] \ No newline at end of file +[assembly: AssemblyInformationalVersion("7.6.0-alpha060")] \ No newline at end of file diff --git a/src/Umbraco.Core/Configuration/UmbracoVersion.cs b/src/Umbraco.Core/Configuration/UmbracoVersion.cs index a741f65e4e..a21c9b7bab 100644 --- a/src/Umbraco.Core/Configuration/UmbracoVersion.cs +++ b/src/Umbraco.Core/Configuration/UmbracoVersion.cs @@ -24,7 +24,7 @@ namespace Umbraco.Core.Configuration /// Gets the version comment (like beta or RC). /// /// The version comment. - public static string CurrentComment { get { return "alpha056"; } } + public static string CurrentComment { get { return "alpha060"; } } // Get the version of the umbraco.dll by looking at a class in that dll // Had to do it like this due to medium trust issues, see: http://haacked.com/archive/2010/11/04/assembly-location-and-medium-trust.aspx From 67fc8b6215c9a16d559cd35ab1f75eccc89a552c Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 10 Feb 2017 15:50:31 +0100 Subject: [PATCH 03/12] Scope - fix media files deletion --- src/Umbraco.Core/Events/DeleteEventArgs.cs | 2 +- .../Events/IDeletingMediaFilesEventArgs.cs | 9 +++ .../Events/RecycleBinEventArgs.cs | 4 +- .../Events/ScopeEventDispatcher.cs | 13 ++++ src/Umbraco.Core/IO/MediaFileSystem.cs | 63 ++++++++++++++++++- .../Interfaces/IDeleteMediaFilesRepository.cs | 6 +- src/Umbraco.Core/Services/ContentService.cs | 20 +++--- src/Umbraco.Core/Services/MediaService.cs | 6 -- src/Umbraco.Core/Services/MemberService.cs | 21 +++---- src/Umbraco.Core/Umbraco.Core.csproj | 1 + 10 files changed, 110 insertions(+), 35 deletions(-) create mode 100644 src/Umbraco.Core/Events/IDeletingMediaFilesEventArgs.cs diff --git a/src/Umbraco.Core/Events/DeleteEventArgs.cs b/src/Umbraco.Core/Events/DeleteEventArgs.cs index 8a0fdaf290..df13363b95 100644 --- a/src/Umbraco.Core/Events/DeleteEventArgs.cs +++ b/src/Umbraco.Core/Events/DeleteEventArgs.cs @@ -3,7 +3,7 @@ using System.Collections.Generic; namespace Umbraco.Core.Events { - public class DeleteEventArgs : CancellableObjectEventArgs>, IEquatable> + public class DeleteEventArgs : CancellableObjectEventArgs>, IEquatable>, IDeletingMediaFilesEventArgs { /// /// Constructor accepting multiple entities that are used in the delete operation diff --git a/src/Umbraco.Core/Events/IDeletingMediaFilesEventArgs.cs b/src/Umbraco.Core/Events/IDeletingMediaFilesEventArgs.cs new file mode 100644 index 0000000000..45681042ba --- /dev/null +++ b/src/Umbraco.Core/Events/IDeletingMediaFilesEventArgs.cs @@ -0,0 +1,9 @@ +using System.Collections.Generic; + +namespace Umbraco.Core.Events +{ + internal interface IDeletingMediaFilesEventArgs + { + List MediaFilesToDelete { get; } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Events/RecycleBinEventArgs.cs b/src/Umbraco.Core/Events/RecycleBinEventArgs.cs index c6049cb7a6..c6d7c659b7 100644 --- a/src/Umbraco.Core/Events/RecycleBinEventArgs.cs +++ b/src/Umbraco.Core/Events/RecycleBinEventArgs.cs @@ -5,7 +5,7 @@ using Umbraco.Core.Models; namespace Umbraco.Core.Events { - public class RecycleBinEventArgs : CancellableEventArgs, IEquatable + public class RecycleBinEventArgs : CancellableEventArgs, IEquatable, IDeletingMediaFilesEventArgs { public RecycleBinEventArgs(Guid nodeObjectType, Dictionary> allPropertyData, bool emptiedSuccessfully) : base(false) @@ -97,6 +97,8 @@ namespace Umbraco.Core.Events /// public List Files { get; private set; } + public List MediaFilesToDelete { get { return Files; } } + /// /// Gets the list of all property data associated with a content id /// diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs index 6eb6ee3b85..31331a2d8d 100644 --- a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs +++ b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; +using Umbraco.Core.IO; namespace Umbraco.Core.Events { @@ -117,9 +118,21 @@ namespace Umbraco.Core.Events if (_events == null) return; + var mediaFileSystem = FileSystemProviderManager.Current.MediaFileSystem; + if (RaiseEvents && completed) + { foreach (var e in _events) + { e.RaiseEvent(); + + // fixme - not sure I like doing it here - but then where? how? + var delete = e.Args as IDeletingMediaFilesEventArgs; + if (delete != null && delete.MediaFilesToDelete.Count > 0) + mediaFileSystem.DeleteMediaFiles(delete.MediaFilesToDelete); + } + } + _events.Clear(); } } diff --git a/src/Umbraco.Core/IO/MediaFileSystem.cs b/src/Umbraco.Core/IO/MediaFileSystem.cs index 6f32ef6da0..d9281a7590 100644 --- a/src/Umbraco.Core/IO/MediaFileSystem.cs +++ b/src/Umbraco.Core/IO/MediaFileSystem.cs @@ -7,6 +7,7 @@ using System.Globalization; using System.IO; using System.Linq; using System.Threading; +using System.Threading.Tasks; using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.Logging; @@ -24,6 +25,7 @@ namespace Umbraco.Core.IO { private readonly IContentSection _contentConfig; private readonly UploadAutoFillProperties _uploadAutoFillProperties; + private readonly ILogger _logger; private readonly object _folderCounterLock = new object(); private long _folderCounter; @@ -42,6 +44,7 @@ namespace Umbraco.Core.IO public MediaFileSystem(IFileSystem wrapped, IContentSection contentConfig, ILogger logger) : base(wrapped) { + _logger = logger; _contentConfig = contentConfig; _uploadAutoFillProperties = new UploadAutoFillProperties(this, logger, contentConfig); } @@ -99,7 +102,7 @@ namespace Umbraco.Core.IO } else { - // new scheme: path is "-/" OR "--" + // new scheme: path is "/" where xuid is a combination of cuid and puid // default media filesystem maps to "~/media/" // assumes that cuid and puid keys can be trusted - and that a single property type // for a single content cannot store two different files with the same name @@ -435,6 +438,64 @@ namespace Umbraco.Core.IO } } + public void DeleteMediaFiles(IEnumerable files) + { + files = files.Distinct(); + + Parallel.ForEach(files, file => + { + try + { + if (file.IsNullOrWhiteSpace()) return; + + if (FileExists(file) == false) return; + DeleteFile(file, true); + + if (UseTheNewMediaPathScheme == false) + { + // old scheme: filepath is "/" OR "-" + // remove the directory if any + var dir = Path.GetDirectoryName(file); + if (string.IsNullOrWhiteSpace(dir) == false) + DeleteDirectory(dir, true); + } + else + { + // new scheme: path is "/" where xuid is a combination of cuid and puid + // remove the directory + var dir = Path.GetDirectoryName(file); + DeleteDirectory(dir, true); + } + + // I don't even understand... + /* + + var relativeFilePath = GetRelativePath(file); // fixme - should be relative already + if (FileExists(relativeFilePath) == false) return; + + var parentDirectory = Path.GetDirectoryName(relativeFilePath); + + // don't want to delete the media folder if not using directories. + if (_contentSection.UploadAllowDirectories && parentDirectory != GetRelativePath("/")) + { + //issue U4-771: if there is a parent directory the recursive parameter should be true + DeleteDirectory(parentDirectory, string.IsNullOrEmpty(parentDirectory) == false); + } + else + { + DeleteFile(file, true); + } + + */ + } + catch (Exception e) + { + _logger.Error("Failed to delete attached file \"" + file + "\".", e); + } + }); + } + + #endregion #region GenerateThumbnails diff --git a/src/Umbraco.Core/Persistence/Repositories/Interfaces/IDeleteMediaFilesRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Interfaces/IDeleteMediaFilesRepository.cs index 005c1d62ba..8a5a99b32a 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Interfaces/IDeleteMediaFilesRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Interfaces/IDeleteMediaFilesRepository.cs @@ -1,7 +1,10 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; namespace Umbraco.Core.Persistence.Repositories { + // cannot kill in v7 because it is public, kill in v8 + [Obsolete("Use MediaFileSystem.DeleteMediaFiles instead.", false)] public interface IDeleteMediaFilesRepository { /// @@ -9,6 +12,7 @@ namespace Umbraco.Core.Persistence.Repositories /// /// /// + [Obsolete("Use MediaFileSystem.DeleteMediaFiles instead.", false)] bool DeleteMediaFiles(IEnumerable files); } } \ No newline at end of file diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index af06c7e0ba..2455a53f55 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -179,7 +179,7 @@ namespace Umbraco.Core.Services uow.Events.Dispatch(Created, this, new NewEventArgs(content, false, contentTypeAlias, parentId)); //Created.RaiseEvent(new NewEventArgs(content, false, contentTypeAlias, parentId), this, uow.Events); - // fixme + // fixme var auditRepo = RepositoryFactory.CreateAuditRepository(uow); auditRepo.AddOrUpdate(new AuditItem(content.Id, string.Format("Content '{0}' was created", name), AuditType.New, content.CreatorId)); uow.Commit(); @@ -1051,9 +1051,9 @@ namespace Umbraco.Core.Services UnPublish(content, userId); } content.WriterId = userId; - content.ChangeTrashedState(true); + content.ChangeTrashedState(true); repository.AddOrUpdate(content); - + //Loop through descendants to update their trash state, but ensuring structure by keeping the ParentId foreach (var descendant in descendants) { @@ -1063,14 +1063,14 @@ namespace Umbraco.Core.Services descendant.ChangeTrashedState(true, descendant.ParentId); repository.AddOrUpdate(descendant); - moveInfo.Add(new MoveEventInfo(descendant, descendant.Path, descendant.ParentId)); + moveInfo.Add(new MoveEventInfo(descendant, descendant.Path, descendant.ParentId)); } uow.Commit(); uow.Events.Dispatch(Trashed, this, new MoveEventArgs(false, evtMsgs, moveInfo.ToArray()), "Trashed"); } - + Audit(AuditType.Move, "Move Content to Recycle Bin performed by user", userId, content.Id); return OperationStatus.Success(evtMsgs); @@ -1285,9 +1285,6 @@ namespace Umbraco.Core.Services var args = new DeleteEventArgs(content, false, evtMsgs); uow.Events.Dispatch(Deleted, this, args, "Deleted"); // fixme why the event name?! - - //remove any flagged media files - repository.DeleteMediaFiles(args.MediaFilesToDelete); } Audit(AuditType.Delete, "Delete Content performed by user", userId, content.Id); @@ -1335,7 +1332,7 @@ namespace Umbraco.Core.Services /// /// Deletes all content of the specified types. All Descendants of deleted content that is not of these types is moved to Recycle Bin. - /// + /// /// Id of the /// Optional Id of the user issueing the delete operation public void DeleteContentOfTypes(IEnumerable contentTypeIds, int userId = 0) @@ -1344,7 +1341,7 @@ namespace Umbraco.Core.Services using (var uow = UowProvider.GetUnitOfWork()) { var repository = RepositoryFactory.CreateContentRepository(uow); - + //track the 'root' items of the collection of nodes discovered to delete, we need to use //these items to lookup descendants that are not of this doc type so they can be transfered //to the recycle bin @@ -1559,9 +1556,6 @@ namespace Umbraco.Core.Services success = repository.EmptyRecycleBin(); - if (success) - repository.DeleteMediaFiles(files); - uow.Events.Dispatch(EmptiedRecycleBin, this, new RecycleBinEventArgs(nodeObjectType, entities, files, success)); uow.Commit(); } diff --git a/src/Umbraco.Core/Services/MediaService.cs b/src/Umbraco.Core/Services/MediaService.cs index 9f5ec811c9..08494a3662 100644 --- a/src/Umbraco.Core/Services/MediaService.cs +++ b/src/Umbraco.Core/Services/MediaService.cs @@ -831,9 +831,6 @@ namespace Umbraco.Core.Services var args = new DeleteEventArgs(media, false, evtMsgs); uow.Events.Dispatch(Deleted, this, args); - - //remove any flagged media files - repository.DeleteMediaFiles(args.MediaFilesToDelete); } Audit(AuditType.Delete, "Delete Media performed by user", userId, media.Id); @@ -958,9 +955,6 @@ namespace Umbraco.Core.Services success = repository.EmptyRecycleBin(); // FIXME shouldn't we commit here?! uow.Events.Dispatch(EmptiedRecycleBin, this, new RecycleBinEventArgs(nodeObjectType, entities, files, success)); - - if (success) - repository.DeleteMediaFiles(files); } } Audit(AuditType.Delete, "Empty Media Recycle Bin performed by user", 0, -21); diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index 7c7afdd2a6..841d79f687 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -45,7 +45,7 @@ namespace Umbraco.Core.Services /// /// Gets the default MemberType alias /// - /// By default we'll return the 'writer', but we need to check it exists. If it doesn't we'll + /// By default we'll return the 'writer', but we need to check it exists. If it doesn't we'll /// return the first type that is not an admin, otherwise if there's only one we will return that one. /// Alias of the default MemberType public string GetDefaultMemberType() @@ -86,7 +86,7 @@ namespace Umbraco.Core.Services /// /// This is simply a helper method which essentially just wraps the MembershipProvider's ChangePassword method /// - /// This method exists so that Umbraco developers can use one entry point to create/update + /// This method exists so that Umbraco developers can use one entry point to create/update /// Members if they choose to. /// The Member to save the password for /// The password to encrypt and save @@ -776,7 +776,7 @@ namespace Umbraco.Core.Services /// /// Creates and persists a Member /// - /// Using this method will persist the Member object before its returned + /// Using this method will persist the Member object before its returned /// meaning that it will have an Id available (unlike the CreateMember method) /// Username of the Member to create /// Email of the Member to create @@ -792,7 +792,7 @@ namespace Umbraco.Core.Services /// /// Creates and persists a Member /// - /// Using this method will persist the Member object before its returned + /// Using this method will persist the Member object before its returned /// meaning that it will have an Id available (unlike the CreateMember method) /// Username of the Member to create /// Email of the Member to create @@ -806,7 +806,7 @@ namespace Umbraco.Core.Services /// /// Creates and persists a Member /// - /// Using this method will persist the Member object before its returned + /// Using this method will persist the Member object before its returned /// meaning that it will have an Id available (unlike the CreateMember method) /// Username of the Member to create /// Email of the Member to create @@ -836,7 +836,7 @@ namespace Umbraco.Core.Services /// /// Creates and persists a Member /// - /// Using this method will persist the Member object before its returned + /// Using this method will persist the Member object before its returned /// meaning that it will have an Id available (unlike the CreateMember method) /// Username of the Member to create /// Email of the Member to create @@ -921,7 +921,7 @@ namespace Umbraco.Core.Services /// public IMember GetByUsername(string username) { - //TODO: Somewhere in here, whether at this level or the repository level, we need to add + //TODO: Somewhere in here, whether at this level or the repository level, we need to add // a caching mechanism since this method is used by all the membership providers and could be // called quite a bit when dealing with members. @@ -953,9 +953,6 @@ namespace Umbraco.Core.Services var args = new DeleteEventArgs(member, false); uow.Events.Dispatch(Deleted, this, args); - - //remove any flagged media files - repository.DeleteMediaFiles(args.MediaFilesToDelete); } } @@ -963,7 +960,7 @@ namespace Umbraco.Core.Services /// Saves an /// /// to Save - /// Optional parameter to raise events. + /// Optional parameter to raise events. /// Default is True otherwise set to False to not raise events public void Save(IMember entity, bool raiseEvents = true) { @@ -1004,7 +1001,7 @@ namespace Umbraco.Core.Services /// Saves a list of objects /// /// to save - /// Optional parameter to raise events. + /// Optional parameter to raise events. /// Default is True otherwise set to False to not raise events public void Save(IEnumerable entities, bool raiseEvents = true) { diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 94267cf21f..8565144dee 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -313,6 +313,7 @@ + From 4f9bbdf7f724ba11273a545fd2d3090abccb4cbe Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 6 Feb 2017 18:26:33 +0100 Subject: [PATCH 04/12] Scope - Fix BulkInsertRecords, ensure a db connection is open --- .../Persistence/PetaPocoExtensions.cs | 55 ++++++++++++------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Core/Persistence/PetaPocoExtensions.cs b/src/Umbraco.Core/Persistence/PetaPocoExtensions.cs index 2db9fcfdb4..8626714a43 100644 --- a/src/Umbraco.Core/Persistence/PetaPocoExtensions.cs +++ b/src/Umbraco.Core/Persistence/PetaPocoExtensions.cs @@ -105,8 +105,8 @@ namespace Umbraco.Core.Persistence // try to update var rowCount = updateCommand.IsNullOrWhiteSpace() - ? db.Update(poco) - : db.Update(updateCommand, updateArgs); + ? db.Update(poco) + : db.Update(updateCommand, updateArgs); if (rowCount > 0) return RecordPersistenceType.Update; @@ -162,7 +162,7 @@ namespace Umbraco.Core.Persistence [Obsolete("Use the DatabaseSchemaHelper instead")] public static void CreateTable(this Database db) - where T : new() + where T : new() { var creator = new DatabaseSchemaHelper(db, LoggerResolver.Current.Logger, SqlSyntaxContext.SqlSyntaxProvider); creator.CreateTable(); @@ -170,7 +170,7 @@ namespace Umbraco.Core.Persistence [Obsolete("Use the DatabaseSchemaHelper instead")] public static void CreateTable(this Database db, bool overwrite) - where T : new() + where T : new() { var creator = new DatabaseSchemaHelper(db, LoggerResolver.Current.Logger, SqlSyntaxContext.SqlSyntaxProvider); creator.CreateTable(overwrite); @@ -231,6 +231,24 @@ namespace Umbraco.Core.Persistence ISqlSyntaxProvider syntaxProvider, bool useNativeSqlPlatformBulkInsert = true, bool commitTrans = false) + { + db.OpenSharedConnection(); + try + { + return BulkInsertRecordsTry(db, collection, tr, syntaxProvider, useNativeSqlPlatformBulkInsert, commitTrans); + } + finally + { + db.CloseSharedConnection(); + } + } + + public static int BulkInsertRecordsTry(this Database db, + IEnumerable collection, + Transaction tr, + ISqlSyntaxProvider syntaxProvider, + bool useNativeSqlPlatformBulkInsert = true, + bool commitTrans = false) { if (commitTrans && tr == null) throw new ArgumentNullException("tr", "The transaction cannot be null if commitTrans is true."); @@ -241,15 +259,15 @@ namespace Umbraco.Core.Persistence return 0; } - var pd = Database.PocoData.ForType(typeof(T)); - if (pd == null) throw new InvalidOperationException("Could not find PocoData for " + typeof(T)); + var pd = Database.PocoData.ForType(typeof (T)); + if (pd == null) throw new InvalidOperationException("Could not find PocoData for " + typeof (T)); try { int processed = 0; var usedNativeSqlPlatformInserts = useNativeSqlPlatformBulkInsert - && NativeSqlPlatformBulkInsertRecords(db, syntaxProvider, pd, collection, out processed); + && NativeSqlPlatformBulkInsertRecords(db, syntaxProvider, pd, collection, out processed); if (usedNativeSqlPlatformInserts == false) { @@ -283,19 +301,13 @@ namespace Umbraco.Core.Persistence } if (commitTrans) - { - if (tr == null) throw new ArgumentNullException("The transaction cannot be null if commitTrans is true"); tr.Complete(); - } return processed; } catch { if (commitTrans) - { - if (tr == null) throw new ArgumentNullException("The transaction cannot be null if commitTrans is true"); tr.Dispose(); - } throw; } @@ -337,13 +349,16 @@ namespace Umbraco.Core.Persistence IEnumerable collection, out string[] sql) { + if (db == null) throw new ArgumentNullException("db"); + if (db.Connection == null) throw new ArgumentException("db.Connection is null."); + var tableName = db.EscapeTableName(pd.TableInfo.TableName); //get all columns to include and format for sql var cols = string.Join(", ", pd.Columns - .Where(c => IncludeColumn(pd, c)) - .Select(c => tableName + "." + db.EscapeSqlIdentifier(c.Key)).ToArray()); + .Where(c => IncludeColumn(pd, c)) + .Select(c => tableName + "." + db.EscapeSqlIdentifier(c.Key)).ToArray()); var itemArray = collection.ToArray(); @@ -357,9 +372,9 @@ namespace Umbraco.Core.Persistence // 4168 / 262 = 15.908... = there will be 16 trans in total //all items will be included if we have disabled db parameters - var itemsPerTrans = Math.Floor(2000.00 / paramsPerItem); + var itemsPerTrans = Math.Floor(2000.00/paramsPerItem); //there will only be one transaction if we have disabled db parameters - var numTrans = Math.Ceiling(itemArray.Length / itemsPerTrans); + var numTrans = Math.Ceiling(itemArray.Length/itemsPerTrans); var sqlQueries = new List(); var commands = new List(); @@ -367,8 +382,8 @@ namespace Umbraco.Core.Persistence for (var tIndex = 0; tIndex < numTrans; tIndex++) { var itemsForTrans = itemArray - .Skip(tIndex * (int)itemsPerTrans) - .Take((int)itemsPerTrans); + .Skip(tIndex*(int) itemsPerTrans) + .Take((int) itemsPerTrans); var cmd = db.CreateCommand(db.Connection, string.Empty); var pocoValues = new List(); @@ -418,7 +433,6 @@ namespace Umbraco.Core.Persistence /// The number of records inserted private static bool NativeSqlPlatformBulkInsertRecords(Database db, ISqlSyntaxProvider syntaxProvider, Database.PocoData pd, IEnumerable collection, out int processed) { - var dbConnection = db.Connection; //unwrap the profiled connection if there is one @@ -447,7 +461,6 @@ namespace Umbraco.Core.Persistence //could not use the SQL server's specific bulk insert operations processed = 0; return false; - } /// From 74e8390bdaf662f84d07b92bb843a8dae7697d00 Mon Sep 17 00:00:00 2001 From: Stephan Date: Sat, 11 Feb 2017 09:54:54 +0100 Subject: [PATCH 05/12] Scope - enable http vs call context switching --- src/Umbraco.Core/Scoping/IScopeInternal.cs | 1 + src/Umbraco.Core/Scoping/IScopeProvider.cs | 6 +- src/Umbraco.Core/Scoping/NoScope.cs | 6 +- src/Umbraco.Core/Scoping/Scope.cs | 45 ++++-- src/Umbraco.Core/Scoping/ScopeProvider.cs | 147 ++++++++++++------ .../TestHelpers/BaseDatabaseFactoryTest.cs | 2 +- 6 files changed, 141 insertions(+), 66 deletions(-) diff --git a/src/Umbraco.Core/Scoping/IScopeInternal.cs b/src/Umbraco.Core/Scoping/IScopeInternal.cs index 86276fc7f9..6b2204bfbe 100644 --- a/src/Umbraco.Core/Scoping/IScopeInternal.cs +++ b/src/Umbraco.Core/Scoping/IScopeInternal.cs @@ -7,6 +7,7 @@ namespace Umbraco.Core.Scoping internal interface IScopeInternal : IScope { IScopeInternal ParentScope { get; } + bool CallContext { get; } EventsDispatchMode DispatchMode { get; } IsolationLevel IsolationLevel { get; } UmbracoDatabase DatabaseOrNull { get; } diff --git a/src/Umbraco.Core/Scoping/IScopeProvider.cs b/src/Umbraco.Core/Scoping/IScopeProvider.cs index 6833eac699..b47beb2475 100644 --- a/src/Umbraco.Core/Scoping/IScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/IScopeProvider.cs @@ -24,7 +24,8 @@ namespace Umbraco.Core.Scoping IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null); + bool? scopeFileSystems = null, + bool callContext = false); /// /// Creates a detached scope. @@ -44,10 +45,11 @@ namespace Umbraco.Core.Scoping /// Attaches a scope. /// /// The scope to attach. + /// A value indicating whether to force usage of call context. /// /// Only a scope created by can be attached. /// - void AttachScope(IScope scope); + void AttachScope(IScope scope, bool callContext = false); /// /// Detaches a scope. diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index b08f10ad0e..b77fb715d2 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -29,6 +29,9 @@ namespace Umbraco.Core.Scoping public Guid InstanceId { get { return _instanceId; } } #endif + /// + public bool CallContext { get { return false; } } + /// public RepositoryCacheMode RepositoryCacheMode { @@ -100,8 +103,7 @@ namespace Umbraco.Core.Scoping if (_database != null) _database.Dispose(); - _scopeProvider.AmbientScope = null; - _scopeProvider.AmbientContext = null; + _scopeProvider.SetAmbient(null); _disposed = true; GC.SuppressFinalize(this); diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 824ccd6d80..bcb62cad90 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -19,6 +19,7 @@ namespace Umbraco.Core.Scoping private readonly EventsDispatchMode _dispatchMode; private readonly bool? _scopeFileSystem; private readonly ScopeContext _scopeContext; + private bool _callContext; private bool _disposed; private bool? _completed; @@ -36,7 +37,8 @@ namespace Umbraco.Core.Scoping IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null) + bool? scopeFileSystems = null, + bool callContext = false) { _scopeProvider = scopeProvider; _scopeContext = scopeContext; @@ -44,6 +46,7 @@ namespace Umbraco.Core.Scoping _repositoryCacheMode = repositoryCacheMode; _dispatchMode = dispatchMode; _scopeFileSystem = scopeFileSystems; + _callContext = callContext; Detachable = detachable; #if DEBUG_SCOPES @@ -98,20 +101,20 @@ namespace Umbraco.Core.Scoping IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null) - : this(scopeProvider, null, scopeContext, detachable, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems) - { - } + bool? scopeFileSystems = null, + bool callContext = false) + : this(scopeProvider, null, scopeContext, detachable, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) + { } // initializes a new scope in a nested scopes chain, with its parent public Scope(ScopeProvider scopeProvider, Scope parent, IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null) - : this(scopeProvider, parent, null, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems) - { - } + bool? scopeFileSystems = null, + bool callContext = false) + : this(scopeProvider, parent, null, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) + { } // initializes a new scope, replacing a NoScope instance public Scope(ScopeProvider scopeProvider, NoScope noScope, @@ -119,8 +122,9 @@ namespace Umbraco.Core.Scoping IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null) - : this(scopeProvider, null, scopeContext, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems) + bool? scopeFileSystems = null, + bool callContext = false) + : this(scopeProvider, null, scopeContext, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) { // steal everything from NoScope _database = noScope.DatabaseOrNull; @@ -135,6 +139,18 @@ namespace Umbraco.Core.Scoping public Guid InstanceId { get { return _instanceId; } } #endif + // a value indicating whether to force call-context + public bool CallContext + { + get + { + if (_callContext) return true; + if (ParentScope != null) return ParentScope.CallContext; + return false; + } + set { _callContext = value; } + } + public bool ScopedFileSystems { get @@ -331,7 +347,7 @@ namespace Umbraco.Core.Scoping #endif var parent = ParentScope; - _scopeProvider.AmbientScope = parent; + _scopeProvider.SetAmbientScope(parent); if (parent != null) parent.ChildCompleted(_completed); @@ -418,7 +434,7 @@ namespace Umbraco.Core.Scoping } finally { - _scopeProvider.AmbientContext = null; + _scopeProvider.SetAmbient(null); } } }, () => @@ -426,8 +442,7 @@ namespace Umbraco.Core.Scoping if (Detachable) { // get out of the way, restore original - _scopeProvider.AmbientScope = OrigScope; - _scopeProvider.AmbientContext = OrigContext; + _scopeProvider.SetAmbient(OrigScope, OrigContext); } }); } diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 2f00e5b862..327e9fad8f 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -1,7 +1,5 @@ using System; -using System.Collections.Generic; using System.Data; -using System.Linq; using System.Runtime.Remoting.Messaging; using System.Web; using Umbraco.Core.Events; @@ -24,17 +22,17 @@ namespace Umbraco.Core.Scoping SafeCallContext.Register( () => { - var scope = StaticAmbientScope; - var context = StaticAmbientContext; - StaticAmbientScope = null; - StaticAmbientContext = null; + var scope = AmbientContextScope; + var context = AmbientContextContext; + AmbientContextScope = null; + AmbientContextContext = null; return Tuple.Create(scope, context); }, o => { // cannot re-attached over leaked scope/context // except of course over NoScope (which leaks) - var ambientScope = StaticAmbientScope; + var ambientScope = AmbientContextScope; if (ambientScope != null) { var ambientNoScope = ambientScope as NoScope; @@ -44,11 +42,11 @@ namespace Umbraco.Core.Scoping // this should rollback any pending transaction ambientNoScope.Dispose(); } - if (StaticAmbientContext != null) throw new Exception("Found leaked context when restoring call context."); + if (AmbientContextContext != null) throw new Exception("Found leaked context when restoring call context."); var t = (Tuple)o; - StaticAmbientScope = t.Item1; - StaticAmbientContext = t.Item2; + AmbientContextScope = t.Item1; + AmbientContextContext = t.Item2; }); } @@ -58,7 +56,7 @@ namespace Umbraco.Core.Scoping internal const string ContextItemKey = "Umbraco.Core.Scoping.ScopeContext"; - private static ScopeContext CallContextContextValue + private static ScopeContext CallContextContext { get { return (ScopeContext)CallContext.LogicalGetData(ContextItemKey); } set @@ -68,7 +66,7 @@ namespace Umbraco.Core.Scoping } } - private static ScopeContext HttpContextContextValue + private static ScopeContext HttpContextContext { get { return (ScopeContext)HttpContext.Current.Items[ContextItemKey]; } set @@ -80,23 +78,27 @@ namespace Umbraco.Core.Scoping } } - private static ScopeContext StaticAmbientContext + private static ScopeContext AmbientContextContext { - get { return HttpContext.Current == null ? CallContextContextValue : HttpContextContextValue; } + get + { + // try http context, fallback onto call context + var value = HttpContext.Current == null ? null : HttpContextContext; + return value ?? CallContextContext; + } set { - if (HttpContext.Current == null) - CallContextContextValue = value; - else - HttpContextContextValue = value; + // clear both + if (HttpContext.Current != null) + HttpContextContext = value; + CallContextContext = value; } } /// public ScopeContext AmbientContext { - get { return StaticAmbientContext; } - set { StaticAmbientContext = value; } + get { return AmbientContextContext; } } #endregion @@ -109,7 +111,7 @@ namespace Umbraco.Core.Scoping // only 1 instance which can be disposed and disposed again private static readonly ScopeReference StaticScopeReference = new ScopeReference(new ScopeProvider(null)); - private static IScopeInternal CallContextValue + private static IScopeInternal CallContextScope { get { return (IScopeInternal) CallContext.LogicalGetData(ScopeItemKey); } set @@ -125,7 +127,7 @@ namespace Umbraco.Core.Scoping } } - private static IScopeInternal HttpContextValue + private static IScopeInternal HttpContextScope { get { return (IScopeInternal) HttpContext.Current.Items[ScopeItemKey]; } set @@ -150,33 +152,79 @@ namespace Umbraco.Core.Scoping } } - private static IScopeInternal StaticAmbientScope + private static IScopeInternal AmbientContextScope { - get { return HttpContext.Current == null ? CallContextValue : HttpContextValue; } + get + { + // try http context, fallback onto call context + var value = HttpContext.Current == null ? null : HttpContextScope; + return value ?? CallContextScope; + } set { - if (HttpContext.Current == null) - CallContextValue = value; - else - HttpContextValue = value; + // clear both + if (HttpContext.Current != null) + HttpContextScope = value; + CallContextScope = value; } } /// public IScopeInternal AmbientScope { - get { return StaticAmbientScope; } - set { StaticAmbientScope = value; } + get { return AmbientContextScope; } + } + + public void SetAmbientScope(IScopeInternal value) + { + if (value != null && value.CallContext) + { + if (HttpContext.Current != null) + HttpContextScope = null; // clear http context + CallContextScope = value; // set call context + } + else + { + CallContextScope = null; // clear call context + AmbientContextScope = value; // set appropriate context (maybe null) + } } /// public IScopeInternal GetAmbientOrNoScope() { - return AmbientScope ?? (AmbientScope = new NoScope(this)); + return AmbientScope ?? (AmbientContextScope = new NoScope(this)); } #endregion + public void SetAmbient(IScopeInternal scope, ScopeContext context = null) + { + if (scope != null && scope.CallContext) + { + // clear http context + if (HttpContext.Current != null) + { + HttpContextScope = null; + HttpContextContext = null; + } + + // set call context + CallContextScope = scope; + CallContextContext = context; + } + else + { + // clear call context + CallContextScope = null; + CallContextContext = null; + + // set appropriate context (maybe null) + AmbientContextScope = scope; + AmbientContextContext = context; + } + } + /// public IScope CreateDetachedScope( IsolationLevel isolationLevel = IsolationLevel.Unspecified, @@ -188,7 +236,7 @@ namespace Umbraco.Core.Scoping } /// - public void AttachScope(IScope other) + public void AttachScope(IScope other, bool callContext = false) { var otherScope = other as Scope; if (otherScope == null) @@ -199,8 +247,9 @@ namespace Umbraco.Core.Scoping otherScope.OrigScope = AmbientScope; otherScope.OrigContext = AmbientContext; - AmbientScope = otherScope; - AmbientContext = otherScope.Context; + + otherScope.CallContext = callContext; + SetAmbient(otherScope, otherScope.Context); } /// @@ -221,8 +270,7 @@ namespace Umbraco.Core.Scoping if (scope.Detachable == false) throw new InvalidOperationException("Ambient scope is not detachable."); - AmbientScope = scope.OrigScope; - AmbientContext = scope.OrigContext; + SetAmbient(scope.OrigScope, scope.OrigContext); scope.OrigScope = null; scope.OrigContext = null; return scope; @@ -233,15 +281,18 @@ namespace Umbraco.Core.Scoping IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, - bool? scopeFileSystems = null) + bool? scopeFileSystems = null, + bool callContext = false) { var ambient = AmbientScope; if (ambient == null) { - var context = AmbientContext == null ? new ScopeContext() : null; - var scope = new Scope(this, false, context, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems); - if (AmbientContext == null) AmbientContext = context; // assign only if scope creation did not throw! - return AmbientScope = scope; + var ambientContext = AmbientContext; + var newContext = ambientContext == null ? new ScopeContext() : null; + var scope = new Scope(this, false, newContext, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + // assign only if scope creation did not throw! + SetAmbient(scope, newContext ?? ambientContext); + return scope; } // replace noScope with a real one @@ -255,16 +306,20 @@ namespace Umbraco.Core.Scoping var database = noScope.DatabaseOrNull; if (database != null && database.InTransaction) throw new Exception("NoScope is in a transaction."); - var context = AmbientContext == null ? new ScopeContext() : null; - var scope = new Scope(this, noScope, context, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems); - if (AmbientContext == null) AmbientContext = context; // assign only if scope creation did not throw! - return AmbientScope = scope; + var ambientContext = AmbientContext; + var newContext = ambientContext == null ? new ScopeContext() : null; + var scope = new Scope(this, noScope, newContext, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + // assign only if scope creation did not throw! + SetAmbient(scope, newContext ?? ambientContext); + return scope; } var ambientScope = ambient as Scope; if (ambientScope == null) throw new Exception("Ambient scope is not a Scope instance."); - return AmbientScope = new Scope(this, ambientScope, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems); + var nested = new Scope(this, ambientScope, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + SetAmbient(nested, AmbientContext); + return nested; } /// diff --git a/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs b/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs index 70f48b12fb..43e1ae679b 100644 --- a/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs +++ b/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs @@ -76,7 +76,7 @@ namespace Umbraco.Tests.TestHelpers var scopeProvider = new ScopeProvider(null); if (scopeProvider.AmbientScope != null) scopeProvider.AmbientScope.Dispose(); - scopeProvider.AmbientScope = null; + scopeProvider.SetAmbientScope(null); base.Initialize(); From c64e3a8908b17f9ab9d56dc1bd7581978f2dc392 Mon Sep 17 00:00:00 2001 From: Stephan Date: Sat, 11 Feb 2017 11:54:22 +0100 Subject: [PATCH 06/12] Scope - refactor event dispatching --- src/Umbraco.Core/Events/EventsDispatchMode.cs | 29 ---- .../Events/ScopeEventDispatcher.cs | 145 ++++-------------- .../Events/ScopeEventDispatcherBase.cs | 101 ++++++++++++ src/Umbraco.Core/Scoping/ActionTime.cs | 13 -- src/Umbraco.Core/Scoping/IScopeInternal.cs | 1 - src/Umbraco.Core/Scoping/IScopeProvider.cs | 4 +- src/Umbraco.Core/Scoping/NoScope.cs | 1 - src/Umbraco.Core/Scoping/Scope.cs | 35 ++--- src/Umbraco.Core/Scoping/ScopeProvider.cs | 12 +- src/Umbraco.Core/Umbraco.Core.csproj | 3 +- .../Scoping/ScopeEventDispatcherTests.cs | 95 +++++------- src/Umbraco.Tests/Scoping/ScopedXmlTests.cs | 8 +- 12 files changed, 188 insertions(+), 259 deletions(-) delete mode 100644 src/Umbraco.Core/Events/EventsDispatchMode.cs create mode 100644 src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs delete mode 100644 src/Umbraco.Core/Scoping/ActionTime.cs diff --git a/src/Umbraco.Core/Events/EventsDispatchMode.cs b/src/Umbraco.Core/Events/EventsDispatchMode.cs deleted file mode 100644 index 23fe013af9..0000000000 --- a/src/Umbraco.Core/Events/EventsDispatchMode.cs +++ /dev/null @@ -1,29 +0,0 @@ -namespace Umbraco.Core.Events -{ - public enum EventsDispatchMode - { - // in 7.5 we'd do: - // - // using (var uow = ...) - // { ... } - // Done.RaiseEvent(...); - // - // and so the event would trigger only *after* the transaction has completed, - // so actually PassThrough is more aggressive than what we had in 7.5 and should - // not be used. now in 7.6 we do: - // - // using (var uow = ...) - // { - // ... - // uow.Events.Dispatch(Done, ...); - // } - // - // so the event can be collected, so the default "kinda compatible" more has to be - // the Scope mode, and Passive is for Deploy only - - Unspecified = 0, - PassThrough, // both Doing and Done trigger immediately - Scope, // Doing triggers immediately, Done queued and triggered when & if the scope completes - Passive // Doing never triggers, Done queued and needs to be handled by custom code - } -} \ No newline at end of file diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs index 31331a2d8d..93315a9946 100644 --- a/src/Umbraco.Core/Events/ScopeEventDispatcher.cs +++ b/src/Umbraco.Core/Events/ScopeEventDispatcher.cs @@ -1,139 +1,48 @@ -using System; -using System.Collections.Generic; -using System.Collections.ObjectModel; -using System.Linq; 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 /// /// /// The outer scope is the only scope that can raise events, the inner scope's will defer to the outer scope /// - internal class ScopeEventDispatcher : IEventDispatcher + internal class ScopeEventDispatcher : ScopeEventDispatcherBase { - private readonly EventsDispatchMode _mode; - private List _events; + public ScopeEventDispatcher() + : base(true) + { } - public ScopeEventDispatcher(EventsDispatchMode mode) - { - _mode = mode; - } - - private List Events { get { return _events ?? (_events = new List()); } } - - private bool PassThroughCancelable { get { return _mode == EventsDispatchMode.PassThrough || _mode == EventsDispatchMode.Scope; } } - - private bool PassThrough { get { return _mode == EventsDispatchMode.PassThrough; } } - - private bool RaiseEvents { get { return _mode == EventsDispatchMode.Scope; } } - - public bool DispatchCancelable(EventHandler eventHandler, object sender, CancellableEventArgs args, string eventName = null) - { - if (eventHandler == null) return args.Cancel; - if (PassThroughCancelable == false) return args.Cancel; - eventHandler(sender, args); - return args.Cancel; - } - - public bool DispatchCancelable(EventHandler eventHandler, object sender, TArgs args, string eventName = null) - where TArgs : CancellableEventArgs - { - if (eventHandler == null) return args.Cancel; - if (PassThroughCancelable == false) return args.Cancel; - eventHandler(sender, args); - return args.Cancel; - } - - public bool DispatchCancelable(TypedEventHandler eventHandler, TSender sender, TArgs args, string eventName = null) - where TArgs : CancellableEventArgs - { - if (eventHandler == null) return args.Cancel; - if (PassThroughCancelable == false) return args.Cancel; - eventHandler(sender, args); - return args.Cancel; - } - - public void Dispatch(EventHandler eventHandler, object sender, EventArgs args, string eventName = null) - { - if (eventHandler == null) return; - if (PassThrough) - eventHandler(sender, args); - else - Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); - } - - public void Dispatch(EventHandler eventHandler, object sender, TArgs args, string eventName = null) - { - if (eventHandler == null) return; - if (PassThrough) - eventHandler(sender, args); - else - Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); - } - - public void Dispatch(TypedEventHandler eventHandler, TSender sender, TArgs args, string eventName = null) - { - if (eventHandler == null) return; - if (PassThrough) - eventHandler(sender, args); - else - Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); - } - - public IEnumerable GetEvents(EventDefinitionFilter filter) - { - if (_events == null) - return Enumerable.Empty(); - - switch (filter) - { - case EventDefinitionFilter.All: - return _events; - case EventDefinitionFilter.FirstIn: - var l1 = new OrderedHashSet(); - foreach (var e in _events) - { - l1.Add(e); - } - return l1; - case EventDefinitionFilter.LastIn: - var l2 = new OrderedHashSet(keepOldest:false); - foreach (var e in _events) - { - l2.Add(e); - } - return l2; - default: - throw new ArgumentOutOfRangeException("filter", filter, null); - } - } - - public void ScopeExit(bool completed) + protected override void ScopeExitCompleted() { // fixme - we'd need to de-duplicate events somehow, etc - and the deduplication should be last in wins - if (_events == null) return; - - var mediaFileSystem = FileSystemProviderManager.Current.MediaFileSystem; - - if (RaiseEvents && completed) + foreach (var e in GetEvents(EventDefinitionFilter.All)) { - foreach (var e in _events) - { - e.RaiseEvent(); + e.RaiseEvent(); - // fixme - not sure I like doing it here - but then where? how? - var delete = e.Args as IDeletingMediaFilesEventArgs; - if (delete != null && delete.MediaFilesToDelete.Count > 0) - mediaFileSystem.DeleteMediaFiles(delete.MediaFilesToDelete); + // fixme - not sure I like doing it here - but then where? how? + var delete = e.Args as IDeletingMediaFilesEventArgs; + if (delete != null && delete.MediaFilesToDelete.Count > 0) + MediaFileSystem.DeleteMediaFiles(delete.MediaFilesToDelete); + } + } + + private MediaFileSystem _mediaFileSystem; + + private MediaFileSystem MediaFileSystem + { + get + { + if (_mediaFileSystem != null) return _mediaFileSystem; + + // fixme - insane! reading config goes cross AppDomain and serializes context? + using (new SafeCallContext()) + { + return _mediaFileSystem = FileSystemProviderManager.Current.MediaFileSystem; } } - - _events.Clear(); } } } \ No newline at end of file diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs new file mode 100644 index 0000000000..d8462d18b3 --- /dev/null +++ b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs @@ -0,0 +1,101 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Umbraco.Core.Events +{ + public abstract class ScopeEventDispatcherBase : IEventDispatcher + { + private List _events; + private readonly bool _raiseCancelable; + + protected ScopeEventDispatcherBase(bool raiseCancelable) + { + _raiseCancelable = raiseCancelable; + } + + private List Events { get { return _events ?? (_events = new List()); } } + + public bool DispatchCancelable(EventHandler eventHandler, object sender, CancellableEventArgs args, string eventName = null) + { + if (eventHandler == null) return args.Cancel; + if (_raiseCancelable == false) return args.Cancel; + eventHandler(sender, args); + return args.Cancel; + } + + public bool DispatchCancelable(EventHandler eventHandler, object sender, TArgs args, string eventName = null) + where TArgs : CancellableEventArgs + { + if (eventHandler == null) return args.Cancel; + if (_raiseCancelable == false) return args.Cancel; + eventHandler(sender, args); + return args.Cancel; + } + + public bool DispatchCancelable(TypedEventHandler eventHandler, TSender sender, TArgs args, string eventName = null) + where TArgs : CancellableEventArgs + { + if (eventHandler == null) return args.Cancel; + if (_raiseCancelable == false) return args.Cancel; + eventHandler(sender, args); + return args.Cancel; + } + + public void Dispatch(EventHandler eventHandler, object sender, EventArgs args, string eventName = null) + { + if (eventHandler == null) return; + Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); + } + + public void Dispatch(EventHandler eventHandler, object sender, TArgs args, string eventName = null) + { + if (eventHandler == null) return; + Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); + } + + public void Dispatch(TypedEventHandler eventHandler, TSender sender, TArgs args, string eventName = null) + { + if (eventHandler == null) return; + Events.Add(new EventDefinition(eventHandler, sender, args, eventName)); + } + + public IEnumerable GetEvents(EventDefinitionFilter filter) + { + if (_events == null) + return Enumerable.Empty(); + + switch (filter) + { + case EventDefinitionFilter.All: + return _events; + case EventDefinitionFilter.FirstIn: + var l1 = new OrderedHashSet(); + foreach (var e in _events) + { + l1.Add(e); + } + return l1; + case EventDefinitionFilter.LastIn: + var l2 = new OrderedHashSet(keepOldest: false); + foreach (var e in _events) + { + l2.Add(e); + } + return l2; + default: + throw new ArgumentOutOfRangeException("filter", filter, null); + } + } + + public void ScopeExit(bool completed) + { + if (_events == null) return; + if (completed) + ScopeExitCompleted(); + _events.Clear(); + } + + protected abstract void ScopeExitCompleted(); + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Scoping/ActionTime.cs b/src/Umbraco.Core/Scoping/ActionTime.cs deleted file mode 100644 index 7cb5e3ed35..0000000000 --- a/src/Umbraco.Core/Scoping/ActionTime.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace Umbraco.Core.Scoping -{ - [Flags] - public enum ActionTime - { - None = 0, - BeforeCommit = 1, - BeforeEvents = 2, - BeforeDispose = 4 - } -} \ No newline at end of file diff --git a/src/Umbraco.Core/Scoping/IScopeInternal.cs b/src/Umbraco.Core/Scoping/IScopeInternal.cs index 6b2204bfbe..c1c28b41fe 100644 --- a/src/Umbraco.Core/Scoping/IScopeInternal.cs +++ b/src/Umbraco.Core/Scoping/IScopeInternal.cs @@ -8,7 +8,6 @@ namespace Umbraco.Core.Scoping { IScopeInternal ParentScope { get; } bool CallContext { get; } - EventsDispatchMode DispatchMode { get; } IsolationLevel IsolationLevel { get; } UmbracoDatabase DatabaseOrNull { get; } EventMessages MessagesOrNull { get; } diff --git a/src/Umbraco.Core/Scoping/IScopeProvider.cs b/src/Umbraco.Core/Scoping/IScopeProvider.cs index b47beb2475..9e6bd6fc78 100644 --- a/src/Umbraco.Core/Scoping/IScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/IScopeProvider.cs @@ -23,7 +23,7 @@ namespace Umbraco.Core.Scoping IScope CreateScope( IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false); @@ -38,7 +38,7 @@ namespace Umbraco.Core.Scoping IScope CreateDetachedScope( IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null); /// diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index b77fb715d2..c2d89ff632 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -110,7 +110,6 @@ namespace Umbraco.Core.Scoping } public IScopeInternal ParentScope { get { return null; } } - public EventsDispatchMode DispatchMode { get {return EventsDispatchMode.Unspecified; } } public IsolationLevel IsolationLevel { get {return IsolationLevel.Unspecified; } } public bool ScopedFileSystems { get { return false; } } public void ChildCompleted(bool? completed) { } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index bcb62cad90..f088ca1993 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -16,7 +16,6 @@ namespace Umbraco.Core.Scoping private readonly ScopeProvider _scopeProvider; private readonly IsolationLevel _isolationLevel; private readonly RepositoryCacheMode _repositoryCacheMode; - private readonly EventsDispatchMode _dispatchMode; private readonly bool? _scopeFileSystem; private readonly ScopeContext _scopeContext; private bool _callContext; @@ -36,7 +35,7 @@ namespace Umbraco.Core.Scoping Scope parent, ScopeContext scopeContext, bool detachable, IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false) { @@ -44,7 +43,7 @@ namespace Umbraco.Core.Scoping _scopeContext = scopeContext; _isolationLevel = isolationLevel; _repositoryCacheMode = repositoryCacheMode; - _dispatchMode = dispatchMode; + _eventDispatcher = eventDispatcher; _scopeFileSystem = scopeFileSystems; _callContext = callContext; Detachable = detachable; @@ -77,9 +76,9 @@ namespace Umbraco.Core.Scoping if (repositoryCacheMode != RepositoryCacheMode.Unspecified && parent.RepositoryCacheMode != repositoryCacheMode) throw new ArgumentException("Cannot be different from parent.", "repositoryCacheMode"); - // cannot specify a different mode! - if (_dispatchMode != EventsDispatchMode.Unspecified && parent._dispatchMode != dispatchMode) - throw new ArgumentException("Cannot be different from parent.", "dispatchMode"); + // cannot specify a dispatcher! + if (_eventDispatcher != null) + throw new ArgumentException("Cannot be specified on nested scope.", "eventDispatcher"); // cannot specify a different fs scope! if (scopeFileSystems != null && parent._scopeFileSystem != scopeFileSystems) @@ -100,20 +99,20 @@ namespace Umbraco.Core.Scoping ScopeContext scopeContext, IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false) - : this(scopeProvider, null, scopeContext, detachable, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) + : this(scopeProvider, null, scopeContext, detachable, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext) { } // initializes a new scope in a nested scopes chain, with its parent public Scope(ScopeProvider scopeProvider, Scope parent, IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false) - : this(scopeProvider, parent, null, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) + : this(scopeProvider, parent, null, false, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext) { } // initializes a new scope, replacing a NoScope instance @@ -121,10 +120,10 @@ namespace Umbraco.Core.Scoping ScopeContext scopeContext, IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false) - : this(scopeProvider, null, scopeContext, false, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext) + : this(scopeProvider, null, scopeContext, false, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext) { // steal everything from NoScope _database = noScope.DatabaseOrNull; @@ -160,16 +159,6 @@ namespace Umbraco.Core.Scoping } } - public EventsDispatchMode DispatchMode - { - get - { - if (_dispatchMode != EventsDispatchMode.Unspecified) return _dispatchMode; - if (ParentScope != null) return ParentScope.DispatchMode; - return EventsDispatchMode.Scope; - } - } - /// public RepositoryCacheMode RepositoryCacheMode { @@ -306,7 +295,7 @@ namespace Umbraco.Core.Scoping { EnsureNotDisposed(); if (ParentScope != null) return ParentScope.Events; - return _eventDispatcher ?? (_eventDispatcher = new ScopeEventDispatcher(DispatchMode)); + return _eventDispatcher ?? (_eventDispatcher = new ScopeEventDispatcher()); } } diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 327e9fad8f..66d35106c3 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -229,10 +229,10 @@ namespace Umbraco.Core.Scoping public IScope CreateDetachedScope( IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null) { - return new Scope(this, true, null, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems); + return new Scope(this, true, null, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems); } /// @@ -280,7 +280,7 @@ namespace Umbraco.Core.Scoping public IScope CreateScope( IsolationLevel isolationLevel = IsolationLevel.Unspecified, RepositoryCacheMode repositoryCacheMode = RepositoryCacheMode.Unspecified, - EventsDispatchMode dispatchMode = EventsDispatchMode.Unspecified, + IEventDispatcher eventDispatcher = null, bool? scopeFileSystems = null, bool callContext = false) { @@ -289,7 +289,7 @@ namespace Umbraco.Core.Scoping { var ambientContext = AmbientContext; var newContext = ambientContext == null ? new ScopeContext() : null; - var scope = new Scope(this, false, newContext, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + var scope = new Scope(this, false, newContext, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext); // assign only if scope creation did not throw! SetAmbient(scope, newContext ?? ambientContext); return scope; @@ -308,7 +308,7 @@ namespace Umbraco.Core.Scoping throw new Exception("NoScope is in a transaction."); var ambientContext = AmbientContext; var newContext = ambientContext == null ? new ScopeContext() : null; - var scope = new Scope(this, noScope, newContext, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + var scope = new Scope(this, noScope, newContext, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext); // assign only if scope creation did not throw! SetAmbient(scope, newContext ?? ambientContext); return scope; @@ -317,7 +317,7 @@ namespace Umbraco.Core.Scoping var ambientScope = ambient as Scope; if (ambientScope == null) throw new Exception("Ambient scope is not a Scope instance."); - var nested = new Scope(this, ambientScope, isolationLevel, repositoryCacheMode, dispatchMode, scopeFileSystems, callContext); + var nested = new Scope(this, ambientScope, isolationLevel, repositoryCacheMode, eventDispatcher, scopeFileSystems, callContext); SetAmbient(nested, AmbientContext); return nested; } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 8565144dee..9fcff37763 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -314,6 +314,7 @@ + @@ -362,7 +363,6 @@ - @@ -559,7 +559,6 @@ - diff --git a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs index 7d5754529e..b68e2a3f9f 100644 --- a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Linq; using Moq; using NUnit.Framework; @@ -8,7 +9,6 @@ using Umbraco.Core.Scoping; namespace Umbraco.Tests.Scoping { - [TestFixture] public class ScopeEventDispatcherTests { @@ -21,11 +21,15 @@ namespace Umbraco.Tests.Scoping DoThing3 = null; } - [TestCase(EventsDispatchMode.PassThrough, true, true)] - [TestCase(EventsDispatchMode.PassThrough, true, false)] - [TestCase(EventsDispatchMode.PassThrough, false, true)] - [TestCase(EventsDispatchMode.PassThrough, false, false)] - public void PassThroughCancelable(EventsDispatchMode mode, bool cancel, bool complete) + [TestCase(false, true, true)] + [TestCase(false, true, false)] + [TestCase(false, false, true)] + [TestCase(false, false, false)] + [TestCase(true, true, true)] + [TestCase(true, true, false)] + [TestCase(true, false, true)] + [TestCase(true, false, false)] + public void EventsHandling(bool passive, bool cancel, bool complete) { var counter1 = 0; var counter2 = 0; @@ -34,7 +38,7 @@ namespace Umbraco.Tests.Scoping DoThing2 += (sender, args) => { counter2++; }; var scopeProvider = new ScopeProvider(Mock.Of()); - using (var scope = scopeProvider.CreateScope(dispatchMode: mode)) + using (var scope = scopeProvider.CreateScope(eventDispatcher: passive ? new PassiveEventDispatcher() : null)) { var cancelled = scope.Events.DispatchCancelable(DoThing1, this, new SaveEventArgs("test")); if (cancelled == false) @@ -43,22 +47,15 @@ namespace Umbraco.Tests.Scoping scope.Complete(); } - var expected1 = mode == EventsDispatchMode.Passive ? 0 : 1; + var expected1 = passive ? 0 : 1; Assert.AreEqual(expected1, counter1); - var expected2 = -1; - switch (mode) - { - case EventsDispatchMode.PassThrough: - expected2 = cancel ? 0 : 1; - break; - case EventsDispatchMode.Scope: - expected2 = cancel ? 0 : (complete ? 1 : 0); - break; - case EventsDispatchMode.Passive: - expected2 = 0; - break; - } + int expected2; + if (passive) + expected2 = 0; + else + expected2 = cancel ? 0 : (complete ? 1 : 0); + Assert.AreEqual(expected2, counter2); } @@ -70,7 +67,7 @@ namespace Umbraco.Tests.Scoping DoThing3 += OnDoThingFail; var scopeProvider = new ScopeProvider(Mock.Of()); - using (var scope = scopeProvider.CreateScope(dispatchMode: EventsDispatchMode.Passive)) + using (var scope = scopeProvider.CreateScope(eventDispatcher: new PassiveEventDispatcher())) { scope.Events.Dispatch(DoThing1, this, new SaveEventArgs("test")); scope.Events.Dispatch(DoThing2, this, new SaveEventArgs(0)); @@ -94,45 +91,14 @@ namespace Umbraco.Tests.Scoping [TestCase(true)] [TestCase(false)] - public void EventsDispatchmode_PassThrough(bool complete) - { - var counter = 0; - - DoThing1 += (sender, args) => { counter++; }; - DoThing2 += (sender, args) => { counter++; }; - DoThing3 += (sender, args) => { counter++; }; - - var scopeProvider = new ScopeProvider(Mock.Of()); - using (var scope = scopeProvider.CreateScope(dispatchMode: EventsDispatchMode.PassThrough)) - { - scope.Events.Dispatch(DoThing1, this, new SaveEventArgs("test")); - scope.Events.Dispatch(DoThing2, this, new SaveEventArgs(0)); - scope.Events.Dispatch(DoThing3, this, new SaveEventArgs(0)); - - // events have not been queued - Assert.IsEmpty(scope.Events.GetEvents(EventDefinitionFilter.All)); - - // events have been raised - Assert.AreEqual(3, counter); - - if (complete) - scope.Complete(); - } - - // nothing has changed - Assert.AreEqual(3, counter); - } - - [TestCase(true)] - [TestCase(false)] - public void EventsDispatchMode_Passive(bool complete) + public void EventsDispatching_Passive(bool complete) { DoThing1 += OnDoThingFail; DoThing2 += OnDoThingFail; DoThing3 += OnDoThingFail; var scopeProvider = new ScopeProvider(Mock.Of()); - using (var scope = scopeProvider.CreateScope(dispatchMode: EventsDispatchMode.Passive)) + using (var scope = scopeProvider.CreateScope(eventDispatcher: new PassiveEventDispatcher())) { scope.Events.Dispatch(DoThing1, this, new SaveEventArgs("test")); scope.Events.Dispatch(DoThing2, this, new SaveEventArgs(0)); @@ -150,7 +116,7 @@ namespace Umbraco.Tests.Scoping [TestCase(true)] [TestCase(false)] - public void EventsDispatchMode_Scope(bool complete) + public void EventsDispatching_Scope(bool complete) { var counter = 0; IScope ambientScope = null; @@ -170,7 +136,7 @@ namespace Umbraco.Tests.Scoping }; Guid guid; - using (var scope = scopeProvider.CreateScope(dispatchMode: EventsDispatchMode.Scope)) + using (var scope = scopeProvider.CreateScope()) { Assert.IsNotNull(scopeProvider.AmbientContext); guid = scopeProvider.Context.Enlist("value", Guid.NewGuid, (c, o) => { }); @@ -181,6 +147,7 @@ namespace Umbraco.Tests.Scoping // events have been queued Assert.AreEqual(3, scope.Events.GetEvents(EventDefinitionFilter.All).Count()); + Assert.AreEqual(0, counter); if (complete) scope.Complete(); @@ -215,5 +182,17 @@ namespace Umbraco.Tests.Scoping public static event EventHandler> DoThing2; public static event TypedEventHandler> DoThing3; - } + + public class PassiveEventDispatcher : ScopeEventDispatcherBase + { + public PassiveEventDispatcher() + : base(false) + { } + + protected override void ScopeExitCompleted() + { + // do nothing + } + } + } } \ No newline at end of file diff --git a/src/Umbraco.Tests/Scoping/ScopedXmlTests.cs b/src/Umbraco.Tests/Scoping/ScopedXmlTests.cs index 179c924f7b..6267e1dd1c 100644 --- a/src/Umbraco.Tests/Scoping/ScopedXmlTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopedXmlTests.cs @@ -213,12 +213,8 @@ namespace Umbraco.Tests.Scoping var scopeProvider = ApplicationContext.Current.ScopeProvider as IScopeProviderInternal; Assert.IsNotNull(scopeProvider); - if (complete) - // because some event handlers trigger xml refresh with directly uses the DB - Assert.IsNotNull(scopeProvider.AmbientScope); - else - // because nothing happened - Assert.IsNull(scopeProvider.AmbientScope); + // ambient scope may be null, or maybe not, depending on whether the code that + // was called did proper scoped work, or some direct (NoScope) use of the database Assert.IsNull(scopeProvider.AmbientContext); // limited number of clones! From 12dd01dd29381beab6d6cd54da3edb3568afd7b4 Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 13 Feb 2017 09:04:19 +0100 Subject: [PATCH 07/12] Scope - refactor call context --- src/Umbraco.Core/ObjectExtensions.cs | 5 + src/Umbraco.Core/Scoping/IScopeProvider.cs | 5 +- src/Umbraco.Core/Scoping/Scope.cs | 2 +- src/Umbraco.Core/Scoping/ScopeProvider.cs | 332 +++++++++++------- src/Umbraco.Tests/Scoping/LeakTests.cs | 14 +- src/Umbraco.Tests/Scoping/ScopeTests.cs | 53 +++ .../TestHelpers/BaseDatabaseFactoryTest.cs | 7 +- 7 files changed, 288 insertions(+), 130 deletions(-) diff --git a/src/Umbraco.Core/ObjectExtensions.cs b/src/Umbraco.Core/ObjectExtensions.cs index fcdc2f52a6..6a1befaad8 100644 --- a/src/Umbraco.Core/ObjectExtensions.cs +++ b/src/Umbraco.Core/ObjectExtensions.cs @@ -647,5 +647,10 @@ namespace Umbraco.Core return "[GetPropertyValueException]"; } } + + internal static Guid AsGuid(this object value) + { + return value is Guid ? (Guid) value : Guid.Empty; + } } } \ No newline at end of file diff --git a/src/Umbraco.Core/Scoping/IScopeProvider.cs b/src/Umbraco.Core/Scoping/IScopeProvider.cs index 9e6bd6fc78..754fb63aa7 100644 --- a/src/Umbraco.Core/Scoping/IScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/IScopeProvider.cs @@ -1,4 +1,5 @@ -using System.Data; +using System; +using System.Data; using Umbraco.Core.Events; #if DEBUG_SCOPES using System.Collections.Generic; @@ -66,7 +67,9 @@ namespace Umbraco.Core.Scoping ScopeContext Context { get; } #if DEBUG_SCOPES + Dictionary CallContextObjects { get; } IEnumerable ScopeInfos { get; } + ScopeInfo GetScopeInfo(IScope scope); #endif } } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index f088ca1993..4730495ba9 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -336,7 +336,7 @@ namespace Umbraco.Core.Scoping #endif var parent = ParentScope; - _scopeProvider.SetAmbientScope(parent); + _scopeProvider.AmbientScope = parent; if (parent != null) parent.ChildCompleted(_completed); diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 66d35106c3..7101258447 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -1,9 +1,14 @@ using System; +using System.Collections; +using System.Collections.Generic; using System.Data; using System.Runtime.Remoting.Messaging; using System.Web; using Umbraco.Core.Events; using Umbraco.Core.Persistence; +#if DEBUG_SCOPES +using System.Linq; +#endif namespace Umbraco.Core.Scoping { @@ -22,17 +27,17 @@ namespace Umbraco.Core.Scoping SafeCallContext.Register( () => { - var scope = AmbientContextScope; - var context = AmbientContextContext; - AmbientContextScope = null; - AmbientContextContext = null; + var scope = GetCallContextObject(ScopeItemKey); + var context = GetCallContextObject(ContextItemKey); + SetCallContextObject(ScopeItemKey, null); + SetCallContextObject(ContextItemKey, null); return Tuple.Create(scope, context); }, o => { // cannot re-attached over leaked scope/context // except of course over NoScope (which leaks) - var ambientScope = AmbientContextScope; + var ambientScope = AmbientScopeInternal; if (ambientScope != null) { var ambientNoScope = ambientScope as NoScope; @@ -42,63 +47,185 @@ namespace Umbraco.Core.Scoping // this should rollback any pending transaction ambientNoScope.Dispose(); } - if (AmbientContextContext != null) throw new Exception("Found leaked context when restoring call context."); + if (AmbientContextInternal != null) throw new Exception("Found leaked context when restoring call context."); - var t = (Tuple)o; - AmbientContextScope = t.Item1; - AmbientContextContext = t.Item2; + var t = (Tuple) o; + SetCallContextObject(ScopeItemKey, t.Item1); + SetCallContextObject(ContextItemKey, t.Item2); }); } public IDatabaseFactory2 DatabaseFactory { get; private set; } + #region Context + + // objects that go into the logical call context better be serializable else they'll eventually + // cause issues whenever some cross-AppDomain code executes - could be due to ReSharper running + // tests, any other things (see https://msdn.microsoft.com/en-us/library/dn458353(v=vs.110).aspx), + // but we don't want to make all of our objects serializable since they are *not* meant to be + // used in cross-AppDomain scenario anyways. + // in addition, whatever goes into the logical call context is serialized back and forth any + // time cross-AppDomain code executes, so if we put an "object" there, we'll can *another* + // "object" instance - and so we cannot use a random object as a key. + // so what we do is: we register a guid in the call context, and we keep a table mapping those + // guids to the actual objects. the guid serializes back and forth without causing any issue, + // and we can retrieve the actual objects from the table. + // only issue: how are we supposed to clear the table? we can't, really. objects should take + // care of de-registering themselves from context. + // everything we use does, except the NoScope scope, which just stays there + // + // during tests, NoScope can to into call context... nothing much we can do about it + + private static readonly object StaticCallContextObjectsLock = new object(); + private static readonly Dictionary StaticCallContextObjects + = new Dictionary(); + +#if DEBUG_SCOPES + public Dictionary CallContextObjects + { + get + { + lock (StaticCallContextObjectsLock) + { + // capture in a dictionary + return StaticCallContextObjects.ToDictionary(x => x.Key, x => x.Value); + } + } + } +#endif + + private static T GetCallContextObject(string key) + where T : class + { + var objectKey = CallContext.LogicalGetData(key).AsGuid(); + lock (StaticCallContextObjectsLock) + { + object callContextObject; + return StaticCallContextObjects.TryGetValue(objectKey, out callContextObject) ? (T)callContextObject : null; + } + } + + private static void SetCallContextObject(string key, object value) + { +#if DEBUG_SCOPES + // manage the 'context' that contains the scope (null, "http" or "call") + // first, null-register the existing value + var ambientKey = CallContext.LogicalGetData(ScopeItemKey).AsGuid(); + object o = null; + lock (StaticCallContextObjectsLock) + { + if (ambientKey != default (Guid)) + StaticCallContextObjects.TryGetValue(ambientKey, out o); + } + var ambientScope = o as IScope; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "call"); +#endif + if (value == null) + { + var objectKey = CallContext.LogicalGetData(key).AsGuid(); + CallContext.FreeNamedDataSlot(key); + if (objectKey == default (Guid)) return; + lock (StaticCallContextObjectsLock) + { + StaticCallContextObjects.Remove(objectKey); + } + } + else + { + // note - we are *not* detecting an already-existing value + // because our code in this class *always* sets to null before + // setting to a real value + var objectKey = Guid.NewGuid(); + lock (StaticCallContextObjectsLock) + { + StaticCallContextObjects.Add(objectKey, value); + } + CallContext.LogicalSetData(key, objectKey); + } + } + + internal static Func HttpContextItemsGetter { get; set; } + + private static IDictionary HttpContextItems + { + get + { + return HttpContextItemsGetter == null + ? (HttpContext.Current == null ? null : HttpContext.Current.Items) + : HttpContextItemsGetter(); + } + } + + public static T GetHttpContextObject(string key, bool required = true) + where T : class + { + var httpContextItems = HttpContextItems; + if (httpContextItems != null) + return (T)httpContextItems[key]; + if (required) + throw new Exception("HttpContext.Current is null."); + return null; + } + + private static bool SetHttpContextObject(string key, object value, bool required = true) + { + var httpContextItems = HttpContextItems; + if (httpContextItems == null) + { + if (required) + throw new Exception("HttpContext.Current is null."); + return false; + } +#if DEBUG_SCOPES + // manage the 'context' that contains the scope (null, "http" or "call") + // first, null-register the existing value + var ambientScope = (IScope)httpContextItems[ScopeItemKey]; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "http"); +#endif + if (value == null) + httpContextItems.Remove(key); + else + httpContextItems[key] = value; + return true; + } + + #endregion + #region Ambient Context internal const string ContextItemKey = "Umbraco.Core.Scoping.ScopeContext"; - private static ScopeContext CallContextContext - { - get { return (ScopeContext)CallContext.LogicalGetData(ContextItemKey); } - set - { - if (value == null) CallContext.FreeNamedDataSlot(ContextItemKey); - else CallContext.LogicalSetData(ContextItemKey, value); - } - } - - private static ScopeContext HttpContextContext - { - get { return (ScopeContext)HttpContext.Current.Items[ContextItemKey]; } - set - { - if (value == null) - HttpContext.Current.Items.Remove(ContextItemKey); - else - HttpContext.Current.Items[ContextItemKey] = value; - } - } - - private static ScopeContext AmbientContextContext + internal static ScopeContext AmbientContextInternal { get { // try http context, fallback onto call context - var value = HttpContext.Current == null ? null : HttpContextContext; - return value ?? CallContextContext; + var value = GetHttpContextObject(ContextItemKey, false); + return value ?? GetCallContextObject(ContextItemKey); } set { // clear both - if (HttpContext.Current != null) - HttpContextContext = value; - CallContextContext = value; + SetHttpContextObject(ContextItemKey, null, false); + SetCallContextObject(ContextItemKey, null); + if (value == null) return; + + // set http/call context + if (SetHttpContextObject(ContextItemKey, value, false) == false) + SetCallContextObject(ContextItemKey, value); } } /// public ScopeContext AmbientContext { - get { return AmbientContextContext; } + get { return AmbientContextInternal; } } #endregion @@ -111,117 +238,72 @@ namespace Umbraco.Core.Scoping // only 1 instance which can be disposed and disposed again private static readonly ScopeReference StaticScopeReference = new ScopeReference(new ScopeProvider(null)); - private static IScopeInternal CallContextScope - { - get { return (IScopeInternal) CallContext.LogicalGetData(ScopeItemKey); } - set - { -#if DEBUG_SCOPES - // manage the 'context' that contains the scope (null, "http" or "lcc") - var ambientScope = (IScope) CallContext.LogicalGetData(ScopeItemKey); - if (ambientScope != null) RegisterContext(ambientScope, null); - if (value != null) RegisterContext(value, "lcc"); -#endif - if (value == null) CallContext.FreeNamedDataSlot(ScopeItemKey); - else CallContext.LogicalSetData(ScopeItemKey, value); - } - } - - private static IScopeInternal HttpContextScope - { - get { return (IScopeInternal) HttpContext.Current.Items[ScopeItemKey]; } - set - { -#if DEBUG_SCOPES - // manage the 'context' that contains the scope (null, "http" or "lcc") - var ambientScope = (IScope) HttpContext.Current.Items[ScopeItemKey]; - if (ambientScope != null) RegisterContext(ambientScope, null); - if (value != null) RegisterContext(value, "http"); -#endif - if (value == null) - { - HttpContext.Current.Items.Remove(ScopeItemKey); - HttpContext.Current.Items.Remove(ScopeRefItemKey); - } - else - { - HttpContext.Current.Items[ScopeItemKey] = value; - if (HttpContext.Current.Items[ScopeRefItemKey] == null) - HttpContext.Current.Items[ScopeRefItemKey] = StaticScopeReference; - } - } - } - - private static IScopeInternal AmbientContextScope + internal static IScopeInternal AmbientScopeInternal { get { // try http context, fallback onto call context - var value = HttpContext.Current == null ? null : HttpContextScope; - return value ?? CallContextScope; + var value = GetHttpContextObject(ScopeItemKey, false); + return value ?? GetCallContextObject(ScopeItemKey); } set { // clear both - if (HttpContext.Current != null) - HttpContextScope = value; - CallContextScope = value; + SetHttpContextObject(ScopeItemKey, null, false); + SetHttpContextObject(ScopeRefItemKey, null, false); + SetCallContextObject(ScopeItemKey, null); + if (value == null) return; + + // set http/call context + if (value.CallContext == false && SetHttpContextObject(ScopeItemKey, value, false)) + SetHttpContextObject(ScopeRefItemKey, StaticScopeReference); + else + SetCallContextObject(ScopeItemKey, value); } } /// public IScopeInternal AmbientScope { - get { return AmbientContextScope; } - } - - public void SetAmbientScope(IScopeInternal value) - { - if (value != null && value.CallContext) - { - if (HttpContext.Current != null) - HttpContextScope = null; // clear http context - CallContextScope = value; // set call context - } - else - { - CallContextScope = null; // clear call context - AmbientContextScope = value; // set appropriate context (maybe null) - } + get { return AmbientScopeInternal; } + internal set { AmbientScopeInternal = value; } } /// public IScopeInternal GetAmbientOrNoScope() { - return AmbientScope ?? (AmbientContextScope = new NoScope(this)); + return AmbientScope ?? (AmbientScope = new NoScope(this)); } #endregion public void SetAmbient(IScopeInternal scope, ScopeContext context = null) { - if (scope != null && scope.CallContext) + // clear all + SetHttpContextObject(ScopeItemKey, null, false); + SetHttpContextObject(ScopeRefItemKey, null, false); + SetCallContextObject(ScopeItemKey, null); + SetHttpContextObject(ContextItemKey, null, false); + SetCallContextObject(ContextItemKey, null); + if (scope == null) { - // clear http context - if (HttpContext.Current != null) - { - HttpContextScope = null; - HttpContextContext = null; - } + if (context != null) + throw new ArgumentException("Must be null if scope is null.", "context"); + return; + } - // set call context - CallContextScope = scope; - CallContextContext = context; + if (context == null) + throw new ArgumentNullException("context"); + + if (scope.CallContext == false && SetHttpContextObject(ScopeItemKey, scope, false)) + { + SetHttpContextObject(ScopeRefItemKey, StaticScopeReference); + SetHttpContextObject(ContextItemKey, context); } else { - // clear call context - CallContextScope = null; - CallContextContext = null; - - // set appropriate context (maybe null) - AmbientContextScope = scope; - AmbientContextContext = context; + SetCallContextObject(ScopeItemKey, scope); + SetCallContextObject(ContextItemKey, context); } } @@ -377,11 +459,20 @@ namespace Umbraco.Core.Scoping } } + public ScopeInfo GetScopeInfo(IScope scope) + { + lock (StaticScopeInfosLock) + { + return StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + } + } + //private static void Log(string message, UmbracoDatabase database) //{ // LogHelper.Debug(message + " (" + (database == null ? "" : database.InstanceSid) + ")."); //} + // register a scope and capture its ctor stacktrace public void RegisterScope(IScope scope) { lock (StaticScopeInfosLock) @@ -391,7 +482,8 @@ namespace Umbraco.Core.Scoping } } - // 'context' that contains the scope (null, "http" or "lcc") + // register that a scope is in a 'context' + // 'context' that contains the scope (null, "http" or "call") public static void RegisterContext(IScope scope, string context) { lock (StaticScopeInfosLock) diff --git a/src/Umbraco.Tests/Scoping/LeakTests.cs b/src/Umbraco.Tests/Scoping/LeakTests.cs index 1e4fd00feb..55b80062a4 100644 --- a/src/Umbraco.Tests/Scoping/LeakTests.cs +++ b/src/Umbraco.Tests/Scoping/LeakTests.cs @@ -2,6 +2,7 @@ using System.Data; using System.Runtime.Remoting.Messaging; using NUnit.Framework; +using Umbraco.Core; using Umbraco.Core.Persistence; using Umbraco.Core.Scoping; using Umbraco.Tests.TestHelpers; @@ -38,11 +39,14 @@ namespace Umbraco.Tests.Scoping _database.BeginTransaction(); // opens and maintains a connection // the test is leaking a scope with a non-null database - var contextScope = CallContext.LogicalGetData(ScopeProvider.ScopeItemKey); - Assert.IsNotNull(contextScope); - Assert.IsInstanceOf(CallContext.LogicalGetData(ScopeProvider.ScopeItemKey)); - Assert.IsNotNull(((NoScope) contextScope).DatabaseOrNull); - Assert.AreSame(_database, ((NoScope)contextScope).DatabaseOrNull); + var contextGuid = CallContext.LogicalGetData(ScopeProvider.ScopeItemKey).AsGuid(); + Assert.AreNotEqual(Guid.Empty, contextGuid); + + // only if Core.DEBUG_SCOPES are defined + //var contextScope = DatabaseContext.ScopeProvider.CallContextObjects[contextGuid] as NoScope; + //Assert.IsNotNull(contextScope); + //Assert.IsNotNull(contextScope.DatabaseOrNull); + //Assert.AreSame(_database, contextScope.DatabaseOrNull); // save the connection _connection = _database.Connection; diff --git a/src/Umbraco.Tests/Scoping/ScopeTests.cs b/src/Umbraco.Tests/Scoping/ScopeTests.cs index 446b7ea9f0..87884abd26 100644 --- a/src/Umbraco.Tests/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeTests.cs @@ -1,4 +1,9 @@ using System; +using System.Collections; +using System.Collections.Generic; +using System.Runtime.Remoting.Messaging; +using System.Web; +using Moq; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Persistence; @@ -97,6 +102,54 @@ namespace Umbraco.Tests.Scoping Assert.IsNull(scopeProvider.AmbientScope); } + [Test] + public void NestedMigrateScope() + { + var scopeProvider = DatabaseContext.ScopeProvider; + + Assert.IsNull(scopeProvider.AmbientScope); + var httpContextItems = new Hashtable(); + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + try + { + using (var scope = scopeProvider.CreateScope()) + { + Assert.IsInstanceOf(scope); + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(scope, scopeProvider.AmbientScope); + Assert.AreSame(scope, httpContextItems[ScopeProvider.ScopeItemKey]); + + // only if Core.DEBUG_SCOPES are defined + //Assert.IsEmpty(scopeProvider.CallContextObjects); + + using (var nested = scopeProvider.CreateScope(callContext: true)) + { + Assert.IsInstanceOf(nested); + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(nested, scopeProvider.AmbientScope); + Assert.AreSame(scope, ((Scope) nested).ParentScope); + + // it's moved over to call context + Assert.IsNull(httpContextItems[ScopeProvider.ScopeItemKey]); + var callContextKey = CallContext.LogicalGetData(ScopeProvider.ScopeItemKey).AsGuid(); + Assert.AreNotEqual(Guid.Empty, callContextKey); + + // only if Core.DEBUG_SCOPES are defined + //var ccnested = scopeProvider.CallContextObjects[callContextKey]; + //Assert.AreSame(nested, ccnested); + } + + // it's naturally back in http context + Assert.AreSame(scope, httpContextItems[ScopeProvider.ScopeItemKey]); + } + Assert.IsNull(scopeProvider.AmbientScope); + } + finally + { + ScopeProvider.HttpContextItemsGetter = null; + } + } + [Test] public void NestedCreateScopeContext() { diff --git a/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs b/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs index 43e1ae679b..2370ad1dfd 100644 --- a/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs +++ b/src/Umbraco.Tests/TestHelpers/BaseDatabaseFactoryTest.cs @@ -72,11 +72,12 @@ namespace Umbraco.Tests.TestHelpers GetDbProviderName(), Logger); - // fixme - bah - this is needed to reset static properties? Stephen to update this note + // ensure we start tests in a clean state ie without any scope in context + // anything that used a true 'Scope' would have removed it, but there could + // be a rogue 'NoScope' there - and we want to make sure it is gone var scopeProvider = new ScopeProvider(null); if (scopeProvider.AmbientScope != null) - scopeProvider.AmbientScope.Dispose(); - scopeProvider.SetAmbientScope(null); + scopeProvider.AmbientScope.Dispose(); // removes scope from context base.Initialize(); From b58d8677b3e152844914dbd41bf2ee5177a9d166 Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 14 Feb 2017 09:09:05 +0100 Subject: [PATCH 08/12] Scope - bugfixing --- .../Scoping/IInstanceIdentifiable.cs | 9 ++ src/Umbraco.Core/Scoping/IScope.cs | 6 +- src/Umbraco.Core/Scoping/NoScope.cs | 2 - src/Umbraco.Core/Scoping/Scope.cs | 18 ++- src/Umbraco.Core/Scoping/ScopeContext.cs | 5 +- src/Umbraco.Core/Scoping/ScopeProvider.cs | 104 ++++++++++++------ src/Umbraco.Core/Umbraco.Core.csproj | 1 + src/Umbraco.Tests/Scoping/ScopeTests.cs | 86 +++++++++++++-- 8 files changed, 174 insertions(+), 57 deletions(-) create mode 100644 src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs diff --git a/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs new file mode 100644 index 0000000000..4c88e1c1b5 --- /dev/null +++ b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs @@ -0,0 +1,9 @@ +using System; + +namespace Umbraco.Core.Scoping +{ + public interface IInstanceIdentifiable + { + Guid InstanceId { get; } + } +} diff --git a/src/Umbraco.Core/Scoping/IScope.cs b/src/Umbraco.Core/Scoping/IScope.cs index a0d994b2f2..0386401346 100644 --- a/src/Umbraco.Core/Scoping/IScope.cs +++ b/src/Umbraco.Core/Scoping/IScope.cs @@ -8,7 +8,7 @@ namespace Umbraco.Core.Scoping /// /// Represents a scope. /// - public interface IScope : IDisposable + public interface IScope : IDisposable, IInstanceIdentifiable { /// /// Gets the scope database. @@ -39,9 +39,5 @@ namespace Umbraco.Core.Scoping /// Completes the scope. /// void Complete(); - -#if DEBUG_SCOPES - Guid InstanceId { get; } -#endif } } diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index c2d89ff632..a6e265c1f2 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -24,10 +24,8 @@ namespace Umbraco.Core.Scoping #endif } -#if DEBUG_SCOPES private readonly Guid _instanceId = Guid.NewGuid(); public Guid InstanceId { get { return _instanceId; } } -#endif /// public bool CallContext { get { return false; } } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 4730495ba9..dff4a18017 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -133,10 +133,8 @@ namespace Umbraco.Core.Scoping throw new Exception("NoScope instance is not free."); } -#if DEBUG_SCOPES private readonly Guid _instanceId = Guid.NewGuid(); public Guid InstanceId { get { return _instanceId; } } -#endif // a value indicating whether to force call-context public bool CallContext @@ -189,6 +187,8 @@ namespace Umbraco.Core.Scoping // the parent scope (in a nested scopes chain) public IScopeInternal ParentScope { get; set; } + public bool Attached { get; set; } + // the original scope (when attaching a detachable scope) public IScopeInternal OrigScope { get; set; } @@ -329,7 +329,18 @@ namespace Umbraco.Core.Scoping EnsureNotDisposed(); if (this != _scopeProvider.AmbientScope) + { +#if DEBUG_SCOPES + var ambient = _scopeProvider.AmbientScope; + Logging.LogHelper.Debug("Dispose error (" + (ambient == null ? "no" : "other") + " ambient)"); + if (ambient == null) + throw new InvalidOperationException("Not the ambient scope (no ambient scope)."); + var infos = _scopeProvider.GetScopeInfo(ambient); + throw new InvalidOperationException("Not the ambient scope (see current ambient ctor stack trace).\r\n" + infos.CtorStack); +#else throw new InvalidOperationException("Not the ambient scope."); +#endif + } #if DEBUG_SCOPES _scopeProvider.Disposed(this); @@ -432,6 +443,9 @@ namespace Umbraco.Core.Scoping { // get out of the way, restore original _scopeProvider.SetAmbient(OrigScope, OrigContext); + Attached = false; + OrigScope = null; + OrigContext = null; } }); } diff --git a/src/Umbraco.Core/Scoping/ScopeContext.cs b/src/Umbraco.Core/Scoping/ScopeContext.cs index 7503271b5a..cca0be560d 100644 --- a/src/Umbraco.Core/Scoping/ScopeContext.cs +++ b/src/Umbraco.Core/Scoping/ScopeContext.cs @@ -3,7 +3,7 @@ using System.Collections.Generic; namespace Umbraco.Core.Scoping { - public class ScopeContext + public class ScopeContext : IInstanceIdentifiable { private Dictionary _enlisted; @@ -27,6 +27,9 @@ namespace Umbraco.Core.Scoping throw new AggregateException("Exceptions were thrown by listed actions.", exceptions); } + private readonly Guid _instanceId = Guid.NewGuid(); + public Guid InstanceId { get { return _instanceId; } } + private IDictionary Enlisted { get diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 7101258447..c500a633d0 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -37,7 +37,7 @@ namespace Umbraco.Core.Scoping { // cannot re-attached over leaked scope/context // except of course over NoScope (which leaks) - var ambientScope = AmbientScopeInternal; + var ambientScope = GetCallContextObject(ScopeItemKey); if (ambientScope != null) { var ambientNoScope = ambientScope as NoScope; @@ -47,7 +47,8 @@ namespace Umbraco.Core.Scoping // this should rollback any pending transaction ambientNoScope.Dispose(); } - if (AmbientContextInternal != null) throw new Exception("Found leaked context when restoring call context."); + if (GetCallContextObject(ContextItemKey) != null) + throw new Exception("Found leaked context when restoring call context."); var t = (Tuple) o; SetCallContextObject(ScopeItemKey, t.Item1); @@ -101,27 +102,41 @@ namespace Umbraco.Core.Scoping lock (StaticCallContextObjectsLock) { object callContextObject; - return StaticCallContextObjects.TryGetValue(objectKey, out callContextObject) ? (T)callContextObject : null; + if (StaticCallContextObjects.TryGetValue(objectKey, out callContextObject)) + { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("GotObject " + objectKey.ToString("N").Substring(0, 8)); +#endif + return (T) callContextObject; + } +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("MissedObject " + objectKey.ToString("N").Substring(0, 8)); +#endif + return null; } } - private static void SetCallContextObject(string key, object value) + private static void SetCallContextObject(string key, IInstanceIdentifiable value) { #if DEBUG_SCOPES // manage the 'context' that contains the scope (null, "http" or "call") - // first, null-register the existing value - var ambientKey = CallContext.LogicalGetData(ScopeItemKey).AsGuid(); - object o = null; - lock (StaticCallContextObjectsLock) + // only for scopes of course! + if (key == ScopeItemKey) { - if (ambientKey != default (Guid)) - StaticCallContextObjects.TryGetValue(ambientKey, out o); + // first, null-register the existing value + var ambientKey = CallContext.LogicalGetData(ScopeItemKey).AsGuid(); + object o = null; + lock (StaticCallContextObjectsLock) + { + if (ambientKey != default(Guid)) + StaticCallContextObjects.TryGetValue(ambientKey, out o); + } + var ambientScope = o as IScope; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "call"); } - var ambientScope = o as IScope; - if (ambientScope != null) RegisterContext(ambientScope, null); - // then register the new value - var scope = value as IScope; - if (scope != null) RegisterContext(scope, "call"); #endif if (value == null) { @@ -130,6 +145,9 @@ namespace Umbraco.Core.Scoping if (objectKey == default (Guid)) return; lock (StaticCallContextObjectsLock) { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("RemoveObject " + objectKey.ToString("N").Substring(0, 8)); +#endif StaticCallContextObjects.Remove(objectKey); } } @@ -138,9 +156,12 @@ namespace Umbraco.Core.Scoping // note - we are *not* detecting an already-existing value // because our code in this class *always* sets to null before // setting to a real value - var objectKey = Guid.NewGuid(); + var objectKey = value.InstanceId; lock (StaticCallContextObjectsLock) { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("AddObject " + objectKey.ToString("N").Substring(0, 8)); +#endif StaticCallContextObjects.Add(objectKey, value); } CallContext.LogicalSetData(key, objectKey); @@ -181,12 +202,16 @@ namespace Umbraco.Core.Scoping } #if DEBUG_SCOPES // manage the 'context' that contains the scope (null, "http" or "call") - // first, null-register the existing value - var ambientScope = (IScope)httpContextItems[ScopeItemKey]; - if (ambientScope != null) RegisterContext(ambientScope, null); - // then register the new value - var scope = value as IScope; - if (scope != null) RegisterContext(scope, "http"); + // only for scopes of course! + if (key == ScopeItemKey) + { + // first, null-register the existing value + var ambientScope = (IScope)httpContextItems[ScopeItemKey]; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "http"); + } #endif if (value == null) httpContextItems.Remove(key); @@ -195,7 +220,7 @@ namespace Umbraco.Core.Scoping return true; } - #endregion +#endregion #region Ambient Context @@ -292,9 +317,6 @@ namespace Umbraco.Core.Scoping return; } - if (context == null) - throw new ArgumentNullException("context"); - if (scope.CallContext == false && SetHttpContextObject(ScopeItemKey, scope, false)) { SetHttpContextObject(ScopeRefItemKey, StaticScopeReference); @@ -327,6 +349,10 @@ namespace Umbraco.Core.Scoping if (otherScope.Detachable == false) throw new ArgumentException("Not a detachable scope."); + if (otherScope.Attached) + throw new InvalidOperationException("Already attached."); + + otherScope.Attached = true; otherScope.OrigScope = AmbientScope; otherScope.OrigContext = AmbientContext; @@ -355,6 +381,7 @@ namespace Umbraco.Core.Scoping SetAmbient(scope.OrigScope, scope.OrigContext); scope.OrigScope = null; scope.OrigContext = null; + scope.Attached = false; return scope; } @@ -446,7 +473,7 @@ namespace Umbraco.Core.Scoping // all scope instances that are currently beeing tracked private static readonly object StaticScopeInfosLock = new object(); - private static readonly List StaticScopeInfos = new List(); + private static readonly Dictionary StaticScopeInfos = new Dictionary(); public IEnumerable ScopeInfos { @@ -454,7 +481,7 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - return StaticScopeInfos.ToArray(); // capture in an array + return StaticScopeInfos.Values.ToArray(); // capture in an array } } } @@ -463,7 +490,8 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - return StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + ScopeInfo scopeInfo; + return StaticScopeInfos.TryGetValue(scope, out scopeInfo) ? scopeInfo : null; } } @@ -477,8 +505,9 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - if (StaticScopeInfos.Any(x => x.Scope == scope)) throw new Exception("oops: already registered."); - StaticScopeInfos.Add(new ScopeInfo(scope, Environment.StackTrace)); + if (StaticScopeInfos.ContainsKey(scope)) throw new Exception("oops: already registered."); + //Logging.LogHelper.Debug("Register " + scope.InstanceId.ToString("N").Substring(0, 8)); + StaticScopeInfos[scope] = new ScopeInfo(scope, Environment.StackTrace); } } @@ -488,13 +517,17 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + ScopeInfo info; + if (StaticScopeInfos.TryGetValue(scope, out info) == false) info = null; if (info == null) { if (context == null) return; throw new Exception("oops: unregistered scope."); } + //Logging.LogHelper.Debug("Register context " + (context ?? "null") + " for " + scope.InstanceId.ToString("N").Substring(0, 8)); if (context == null) info.NullStack = Environment.StackTrace; + //if (context == null) + // Logging.LogHelper.Debug("STACK\r\n" + info.NullStack); info.Context = context; } } @@ -503,11 +536,12 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); - if (info != null) + if (StaticScopeInfos.ContainsKey(scope)) { // enable this by default - StaticScopeInfos.Remove(info); + //Console.WriteLine("unregister " + scope.InstanceId.ToString("N").Substring(0, 8)); + StaticScopeInfos.Remove(scope); + //Logging.LogHelper.Debug("Remove " + scope.InstanceId.ToString("N").Substring(0, 8)); // instead, enable this to keep *all* scopes // beware, there can be a lot of scopes! diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 9fcff37763..738634c858 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -559,6 +559,7 @@ + diff --git a/src/Umbraco.Tests/Scoping/ScopeTests.cs b/src/Umbraco.Tests/Scoping/ScopeTests.cs index 87884abd26..747e5b1f40 100644 --- a/src/Umbraco.Tests/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeTests.cs @@ -1,15 +1,12 @@ using System; using System.Collections; -using System.Collections.Generic; using System.Runtime.Remoting.Messaging; -using System.Web; -using Moq; +using System.Threading; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Persistence; using Umbraco.Core.Scoping; using Umbraco.Tests.TestHelpers; -using Umbraco.Core.Events; namespace Umbraco.Tests.Scoping { @@ -106,8 +103,8 @@ namespace Umbraco.Tests.Scoping public void NestedMigrateScope() { var scopeProvider = DatabaseContext.ScopeProvider; - Assert.IsNull(scopeProvider.AmbientScope); + var httpContextItems = new Hashtable(); ScopeProvider.HttpContextItemsGetter = () => httpContextItems; try @@ -531,17 +528,82 @@ namespace Umbraco.Tests.Scoping } [Test] - public void CallContextScope() + public void CallContextScope1() { var scopeProvider = DatabaseContext.ScopeProvider; - var scope = scopeProvider.CreateScope(); - Assert.IsNotNull(scopeProvider.AmbientScope); - using (new SafeCallContext()) + using (var scope = scopeProvider.CreateScope()) { - Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + using (new SafeCallContext()) + { + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + using (var newScope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientScope.ParentScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(scope, scopeProvider.AmbientScope); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + + [Test] + public void CallContextScope2() + { + var scopeProvider = DatabaseContext.ScopeProvider; + Assert.IsNull(scopeProvider.AmbientScope); + + var httpContextItems = new Hashtable(); + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + try + { + using (var scope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + using (new SafeCallContext()) + { + // pretend it's another thread + ScopeProvider.HttpContextItemsGetter = null; + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + using (var newScope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientScope.ParentScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + // back to original thread + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + } + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(scope, scopeProvider.AmbientScope); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + finally + { + ScopeProvider.HttpContextItemsGetter = null; } - Assert.IsNotNull(scopeProvider.AmbientScope); - Assert.AreSame(scope, scopeProvider.AmbientScope); } [Test] From 55c5b416938719e716e767669475ee4223e1b10d Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 14 Feb 2017 09:13:44 +0100 Subject: [PATCH 09/12] Scope - report completion --- src/Umbraco.Core/Scoping/IScope.cs | 4 +++- src/Umbraco.Core/Scoping/NoScope.cs | 2 +- src/Umbraco.Core/Scoping/Scope.cs | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Scoping/IScope.cs b/src/Umbraco.Core/Scoping/IScope.cs index 0386401346..4f178b80bc 100644 --- a/src/Umbraco.Core/Scoping/IScope.cs +++ b/src/Umbraco.Core/Scoping/IScope.cs @@ -38,6 +38,8 @@ namespace Umbraco.Core.Scoping /// /// Completes the scope. /// - void Complete(); + /// A value indicating whether the scope has been successfully completed. + /// Can return false if any child scope has not completed. + bool Complete(); } } diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index a6e265c1f2..2d8fa245b0 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -76,7 +76,7 @@ namespace Umbraco.Core.Scoping } /// - public void Complete() + public bool Complete() { throw new NotSupportedException(); } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index dff4a18017..ab66be1bd2 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -300,10 +300,11 @@ namespace Umbraco.Core.Scoping } /// - public void Complete() + public bool Complete() { if (_completed.HasValue == false) _completed = true; + return _completed.Value; } public void Reset() From 507d7dd4f088250d0bd2791b77e313ebe7289bf4 Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 14 Feb 2017 09:17:24 +0100 Subject: [PATCH 10/12] Scope - forgot to complete some scopes --- src/Umbraco.Core/Services/ContentService.cs | 1 - src/Umbraco.Core/Services/ContentTypeService.cs | 6 +++++- src/Umbraco.Core/Services/MemberTypeService.cs | 8 ++++---- src/Umbraco.Web/Scheduling/LogScrubber.cs | 3 ++- src/Umbraco.Web/Scheduling/ScheduledPublishing.cs | 3 ++- .../DataServices/UmbracoContentService.cs | 12 ++++++++---- 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index 2455a53f55..0723431ada 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -1604,7 +1604,6 @@ namespace Umbraco.Core.Services // fixme mess! using (var scope = UowProvider.ScopeProvider.CreateScope()) { - using (var uow = UowProvider.GetUnitOfWork()) { if (uow.Events.DispatchCancelable(Copying, this, new CopyEventArgs(content, copy, parentId))) diff --git a/src/Umbraco.Core/Services/ContentTypeService.cs b/src/Umbraco.Core/Services/ContentTypeService.cs index 43ed68b5a2..32d14f6656 100644 --- a/src/Umbraco.Core/Services/ContentTypeService.cs +++ b/src/Umbraco.Core/Services/ContentTypeService.cs @@ -756,6 +756,7 @@ namespace Umbraco.Core.Services using (var scope = UowProvider.ScopeProvider.CreateScope()) // fixme what a mess { scope.Events.Dispatch(SavedContentType, this, new SaveEventArgs(contentType, false)); + scope.Complete(); } Audit(AuditType.Save, "Save ContentType performed by user", userId, contentType.Id); } @@ -800,8 +801,9 @@ namespace Umbraco.Core.Services using (var scope = UowProvider.ScopeProvider.CreateScope()) // fixme what a mess { scope.Events.Dispatch(SavedContentType, this, new SaveEventArgs(asArray, false)); + scope.Complete(); } - Audit(AuditType.Save, "Save ContentTypes performed by user", userId, -1); + Audit(AuditType.Save, "Save ContentTypes performed by user", userId, -1); } /// @@ -1211,6 +1213,7 @@ namespace Umbraco.Core.Services using (var scope = UowProvider.ScopeProvider.CreateScope()) // fixme what a mess { scope.Events.Dispatch(SavedMediaType, this, new SaveEventArgs(mediaType, false)); + scope.Complete(); } Audit(AuditType.Save, "Save MediaType performed by user", userId, mediaType.Id); } @@ -1255,6 +1258,7 @@ namespace Umbraco.Core.Services using (var scope = UowProvider.ScopeProvider.CreateScope()) // fixme what a mess { scope.Events.Dispatch(SavedMediaType, this, new SaveEventArgs(asArray, false)); + scope.Complete(); } Audit(AuditType.Save, "Save MediaTypes performed by user", userId, -1); } diff --git a/src/Umbraco.Core/Services/MemberTypeService.cs b/src/Umbraco.Core/Services/MemberTypeService.cs index 4634288357..be55a335b3 100644 --- a/src/Umbraco.Core/Services/MemberTypeService.cs +++ b/src/Umbraco.Core/Services/MemberTypeService.cs @@ -17,7 +17,7 @@ namespace Umbraco.Core.Services private readonly IMemberService _memberService; private static readonly ReaderWriterLockSlim Locker = new ReaderWriterLockSlim(); - + public MemberTypeService(IDatabaseUnitOfWorkProvider provider, RepositoryFactory repositoryFactory, ILogger logger, IEventMessagesFactory eventMessagesFactory, IMemberService memberService) : base(provider, repositoryFactory, logger, eventMessagesFactory) @@ -110,7 +110,7 @@ namespace Umbraco.Core.Services public void Save(IEnumerable memberTypes, int userId = 0) { var asArray = memberTypes.ToArray(); - + using (new WriteLock(Locker)) { using (var uow = UowProvider.GetUnitOfWork()) @@ -137,7 +137,7 @@ namespace Umbraco.Core.Services uow.Events.Dispatch(Saved, this, new SaveEventArgs(asArray, false)); } } - + } public void Delete(IMemberType memberType, int userId = 0) @@ -167,7 +167,7 @@ namespace Umbraco.Core.Services public void Delete(IEnumerable memberTypes, int userId = 0) { var asArray = memberTypes.ToArray(); - + using (new WriteLock(Locker)) { using (var scope = UowProvider.ScopeProvider.CreateScope()) diff --git a/src/Umbraco.Web/Scheduling/LogScrubber.cs b/src/Umbraco.Web/Scheduling/LogScrubber.cs index f18f5ad603..760304574d 100644 --- a/src/Umbraco.Web/Scheduling/LogScrubber.cs +++ b/src/Umbraco.Web/Scheduling/LogScrubber.cs @@ -79,10 +79,11 @@ namespace Umbraco.Web.Scheduling // running on a background task, and Log.CleanLogs uses the old SqlHelper, // better wrap in a scope and ensure it's all cleaned up and nothing leaks - using (ApplicationContext.Current.ScopeProvider.CreateScope()) + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) using (DisposableTimer.DebugDuration("Log scrubbing executing", "Log scrubbing complete")) { Log.CleanLogs(GetLogScrubbingMaximumAge(_settings)); + scope.Complete(); } return true; // repeat diff --git a/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs b/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs index 5ff7963f50..d7ed241275 100644 --- a/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs +++ b/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs @@ -85,10 +85,11 @@ namespace Umbraco.Web.Scheduling // running on a background task, requires its own (safe) scope // (GetAuthenticationHeaderValue uses UserService to load the current user, hence requires a database) // (might not need a scope but we don't know really) - using (ApplicationContext.Current.ScopeProvider.CreateScope()) + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) { //pass custom the authorization header request.Headers.Authorization = AdminTokenAuthorizeAttribute.GetAuthenticationHeaderValue(_appContext); + scope.Complete(); } var result = await wc.SendAsync(request, token); diff --git a/src/UmbracoExamine/DataServices/UmbracoContentService.cs b/src/UmbracoExamine/DataServices/UmbracoContentService.cs index cd6acca8eb..b52f60a2f2 100644 --- a/src/UmbracoExamine/DataServices/UmbracoContentService.cs +++ b/src/UmbracoExamine/DataServices/UmbracoContentService.cs @@ -57,7 +57,7 @@ namespace UmbracoExamine.DataServices [Obsolete("This should no longer be used, latest content will be indexed by using the IContentService directly")] public XDocument GetLatestContentByXPath(string xpath) { - using (ApplicationContext.Current.ScopeProvider.CreateScope()) + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) { var xmlContent = XDocument.Parse(""); var rootContent = _applicationContext.Services.ContentService.GetRootContent(); @@ -67,6 +67,7 @@ namespace UmbracoExamine.DataServices xmlContent.Root.Add(c.ToDeepXml(_applicationContext.Services.PackagingService)); } var result = ((IEnumerable)xmlContent.XPathEvaluate(xpath)).Cast(); + scope.Complete(); return result.ToXDocument(); } } @@ -79,9 +80,11 @@ namespace UmbracoExamine.DataServices /// public bool IsProtected(int nodeId, string path) { - using (ApplicationContext.Current.ScopeProvider.CreateScope()) + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) { - return _applicationContext.Services.PublicAccessService.IsProtected(path.EnsureEndsWith("," + nodeId)); + var ret = _applicationContext.Services.PublicAccessService.IsProtected(path.EnsureEndsWith("," + nodeId)); + scope.Complete(); + return ret; } } @@ -92,11 +95,12 @@ namespace UmbracoExamine.DataServices public IEnumerable GetAllUserPropertyNames() { - using (ApplicationContext.Current.ScopeProvider.CreateScope()) + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) { try { var result = _applicationContext.DatabaseContext.Database.Fetch("select distinct alias from cmsPropertyType order by alias"); + scope.Complete(); return result; } catch (Exception ex) From f889b5206c446b31196fa4c6c92218a983582cf5 Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 14 Feb 2017 11:10:09 +0100 Subject: [PATCH 11/12] Scope - handle publishing events --- .../Cache/CacheRefresherEventHandler.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/Umbraco.Web/Cache/CacheRefresherEventHandler.cs b/src/Umbraco.Web/Cache/CacheRefresherEventHandler.cs index 59084f13c4..b1b9039528 100644 --- a/src/Umbraco.Web/Cache/CacheRefresherEventHandler.cs +++ b/src/Umbraco.Web/Cache/CacheRefresherEventHandler.cs @@ -243,6 +243,20 @@ namespace Umbraco.Web.Cache #region Publishing + // IPublishingStrategy (obsolete) events are proxied into ContentService, which works fine when + // events are actually raised, but not when they are handled by HandleEvents, so we have to have + // these proxy methods that are *not* registered against any event *but* used by HandleEvents. + + static void PublishingStrategy_UnPublished(IPublishingStrategy sender, PublishEventArgs e) + { + ContentService_UnPublished(sender, e); + } + + static void PublishingStrategy_Published(IPublishingStrategy sender, PublishEventArgs e) + { + ContentService_Published(sender, e); + } + static void ContentService_UnPublished(IPublishingStrategy sender, PublishEventArgs e) { if (e.PublishedEntities.Any()) From 0074f579706a0407c43bc6ad21e1c7aad1c9e35f Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 14 Feb 2017 11:10:33 +0100 Subject: [PATCH 12/12] Version 7.6-alpha063 --- build/UmbracoVersion.txt | 2 +- src/SolutionInfo.cs | 2 +- src/Umbraco.Core/Configuration/UmbracoVersion.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build/UmbracoVersion.txt b/build/UmbracoVersion.txt index 0a909cc729..cb4b663ecb 100644 --- a/build/UmbracoVersion.txt +++ b/build/UmbracoVersion.txt @@ -1,3 +1,3 @@ # Usage: on line 2 put the release version, on line 3 put the version comment (example: beta) 7.6.0 -alpha062 \ No newline at end of file +alpha063 diff --git a/src/SolutionInfo.cs b/src/SolutionInfo.cs index e7d7b208fc..ac9db0f47a 100644 --- a/src/SolutionInfo.cs +++ b/src/SolutionInfo.cs @@ -12,4 +12,4 @@ using System.Resources; [assembly: AssemblyVersion("1.0.*")] [assembly: AssemblyFileVersion("7.6.0")] -[assembly: AssemblyInformationalVersion("7.6.0-alpha062")] \ No newline at end of file +[assembly: AssemblyInformationalVersion("7.6.0-alpha063")] \ No newline at end of file diff --git a/src/Umbraco.Core/Configuration/UmbracoVersion.cs b/src/Umbraco.Core/Configuration/UmbracoVersion.cs index 9e325c9afd..a80322aa7e 100644 --- a/src/Umbraco.Core/Configuration/UmbracoVersion.cs +++ b/src/Umbraco.Core/Configuration/UmbracoVersion.cs @@ -24,7 +24,7 @@ namespace Umbraco.Core.Configuration /// Gets the version comment (like beta or RC). /// /// The version comment. - public static string CurrentComment { get { return "alpha062"; } } + public static string CurrentComment { get { return "alpha063"; } } // Get the version of the umbraco.dll by looking at a class in that dll // Had to do it like this due to medium trust issues, see: http://haacked.com/archive/2010/11/04/assembly-location-and-medium-trust.aspx