From 5784ea96e751f78af7c405cfdb477829f7feca7c Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 23 Nov 2016 11:26:30 +0100 Subject: [PATCH] UnRevert "Backport SafeCallContext, DefaultDatabaseFactory fixes from 7.6" This reverts commit 8ba6cb3abf6c5fe1ce965b35788c03ab1f60067b. --- .../Packaging/PackageBinaryInspector.cs | 50 ++--- .../Persistence/DefaultDatabaseFactory.cs | 186 +++++++++++++----- src/Umbraco.Core/SafeCallContext.cs | 94 +++++++++ src/Umbraco.Core/Umbraco.Core.csproj | 1 + .../Persistence/DatabaseContextTests.cs | 4 +- .../DataServices/UmbracoContentService.cs | 1 - 6 files changed, 258 insertions(+), 78 deletions(-) create mode 100644 src/Umbraco.Core/SafeCallContext.cs diff --git a/src/Umbraco.Core/Packaging/PackageBinaryInspector.cs b/src/Umbraco.Core/Packaging/PackageBinaryInspector.cs index 9307e74f21..57664b8a83 100644 --- a/src/Umbraco.Core/Packaging/PackageBinaryInspector.cs +++ b/src/Umbraco.Core/Packaging/PackageBinaryInspector.cs @@ -29,20 +29,24 @@ namespace Umbraco.Core.Packaging /// public static IEnumerable ScanAssembliesForTypeReference(IEnumerable assemblys, out string[] errorReport) { - var appDomain = GetTempAppDomain(); - var type = typeof(PackageBinaryInspector); - try + // beware! when toying with domains, use a safe call context! + using (new SafeCallContext()) { - var value = (PackageBinaryInspector)appDomain.CreateInstanceAndUnwrap( - type.Assembly.FullName, - type.FullName); - // do NOT turn PerformScan into static (even if ReSharper says so)! - var result = value.PerformScan(assemblys.ToArray(), out errorReport); - return result; - } - finally - { - AppDomain.Unload(appDomain); + var appDomain = GetTempAppDomain(); + var type = typeof(PackageBinaryInspector); + try + { + var value = (PackageBinaryInspector) appDomain.CreateInstanceAndUnwrap( + type.Assembly.FullName, + type.FullName); + // do NOT turn PerformScan into static (even if ReSharper says so)! + var result = value.PerformScan(assemblys.ToArray(), out errorReport); + return result; + } + finally + { + AppDomain.Unload(appDomain); + } } } @@ -78,7 +82,7 @@ namespace Umbraco.Core.Packaging /// /// Performs the assembly scanning /// - /// + /// /// /// /// @@ -107,7 +111,7 @@ namespace Umbraco.Core.Packaging /// /// Performs the assembly scanning /// - /// + /// /// /// /// @@ -154,7 +158,7 @@ namespace Umbraco.Core.Packaging //get the list of assembly names to compare below var loadedNames = loaded.Select(x => x.GetName().Name).ToArray(); - + //Then load each referenced assembly into the context foreach (var a in loaded) { @@ -170,7 +174,7 @@ namespace Umbraco.Core.Packaging } catch (FileNotFoundException) { - //if an exception occurs it means that a referenced assembly could not be found + //if an exception occurs it means that a referenced assembly could not be found errors.Add( string.Concat("This package references the assembly '", assemblyName.Name, @@ -179,7 +183,7 @@ namespace Umbraco.Core.Packaging } catch (Exception ex) { - //if an exception occurs it means that a referenced assembly could not be found + //if an exception occurs it means that a referenced assembly could not be found errors.Add( string.Concat("This package could not be verified for compatibility. An error occurred while loading a referenced assembly '", assemblyName.Name, @@ -197,7 +201,7 @@ namespace Umbraco.Core.Packaging { //now we need to see if they contain any type 'T' var reflectedAssembly = a; - + try { var found = reflectedAssembly.GetExportedTypes() @@ -210,8 +214,8 @@ namespace Umbraco.Core.Packaging } catch (Exception ex) { - //This is a hack that nobody can seem to get around, I've read everything and it seems that - // this is quite a common thing when loading types into reflection only load context, so + //This is a hack that nobody can seem to get around, I've read everything and it seems that + // this is quite a common thing when loading types into reflection only load context, so // we're just going to ignore this specific one for now var typeLoadEx = ex as TypeLoadException; if (typeLoadEx != null) @@ -232,7 +236,7 @@ namespace Umbraco.Core.Packaging LogHelper.Error("An error occurred scanning package assemblies", ex); } } - + } errorReport = errors.ToArray(); @@ -252,7 +256,7 @@ namespace Umbraco.Core.Packaging var contractType = contractAssemblyLoadFrom.GetExportedTypes() .FirstOrDefault(x => x.FullName == typeof(T).FullName && x.Assembly.FullName == typeof(T).Assembly.FullName); - + if (contractType == null) { throw new InvalidOperationException("Could not find type " + typeof(T) + " in the LoadFrom assemblies"); diff --git a/src/Umbraco.Core/Persistence/DefaultDatabaseFactory.cs b/src/Umbraco.Core/Persistence/DefaultDatabaseFactory.cs index 8b78f290b3..4ad425d9c9 100644 --- a/src/Umbraco.Core/Persistence/DefaultDatabaseFactory.cs +++ b/src/Umbraco.Core/Persistence/DefaultDatabaseFactory.cs @@ -1,6 +1,6 @@ using System; +using System.Runtime.Remoting.Messaging; using System.Web; -using Umbraco.Core.Configuration; using Umbraco.Core.Logging; namespace Umbraco.Core.Persistence @@ -19,13 +19,24 @@ namespace Umbraco.Core.Persistence private readonly ILogger _logger; public string ConnectionString { get; private set; } public string ProviderName { get; private set; } - - //very important to have ThreadStatic: - // see: http://issues.umbraco.org/issue/U4-2172 - [ThreadStatic] - private static volatile UmbracoDatabase _nonHttpInstance; - private static readonly object Locker = new object(); + // NO! see notes in v8 HybridAccessorBase + //[ThreadStatic] + //private static volatile UmbracoDatabase _nonHttpInstance; + + private const string ItemKey = "Umbraco.Core.Persistence.DefaultDatabaseFactory"; + + private static UmbracoDatabase NonContextValue + { + get { return (UmbracoDatabase) CallContext.LogicalGetData(ItemKey); } + set + { + if (value == null) CallContext.FreeNamedDataSlot(ItemKey); + else CallContext.LogicalSetData(ItemKey, value); + } + } + + private static readonly object Locker = new object(); /// /// Constructor accepting custom connection string @@ -36,7 +47,10 @@ namespace Umbraco.Core.Persistence { if (logger == null) throw new ArgumentNullException("logger"); Mandate.ParameterNotNullOrEmpty(connectionStringName, "connectionStringName"); - _connectionStringName = connectionStringName; + + //if (NonContextValue != null) throw new Exception("NonContextValue is not null."); + + _connectionStringName = connectionStringName; _logger = logger; } @@ -51,65 +65,133 @@ namespace Umbraco.Core.Persistence if (logger == null) throw new ArgumentNullException("logger"); Mandate.ParameterNotNullOrEmpty(connectionString, "connectionString"); Mandate.ParameterNotNullOrEmpty(providerName, "providerName"); - ConnectionString = connectionString; + + //if (NonContextValue != null) throw new Exception("NonContextValue is not null."); + + ConnectionString = connectionString; ProviderName = providerName; _logger = logger; } public UmbracoDatabase CreateDatabase() { - //no http context, create the singleton global object - if (HttpContext.Current == null) - { - if (_nonHttpInstance == null) - { - lock (Locker) - { - //double check - if (_nonHttpInstance == null) - { - _nonHttpInstance = string.IsNullOrEmpty(ConnectionString) == false && string.IsNullOrEmpty(ProviderName) == false - ? new UmbracoDatabase(ConnectionString, ProviderName, _logger) - : new UmbracoDatabase(_connectionStringName, _logger); - } - } - } - return _nonHttpInstance; - } + // no http context, create the call context object + // NOTHING is going to track the object and it is the responsibility of the caller to release it! + // using the ReleaseDatabase method. + if (HttpContext.Current == null) + { + LogHelper.Debug("Get NON http [T" + Environment.CurrentManagedThreadId + "]"); + var value = NonContextValue; + if (value != null) return value; + lock (Locker) + { + value = NonContextValue; + if (value != null) return value; - //we have an http context, so only create one per request - if (HttpContext.Current.Items.Contains(typeof(DefaultDatabaseFactory)) == false) - { - HttpContext.Current.Items.Add(typeof (DefaultDatabaseFactory), - string.IsNullOrEmpty(ConnectionString) == false && string.IsNullOrEmpty(ProviderName) == false + LogHelper.Debug("Create NON http [T" + Environment.CurrentManagedThreadId + "]"); + NonContextValue = value = string.IsNullOrEmpty(ConnectionString) == false && string.IsNullOrEmpty(ProviderName) == false + ? new UmbracoDatabase(ConnectionString, ProviderName, _logger) + : new UmbracoDatabase(_connectionStringName, _logger); + + return value; + } + } + + // we have an http context, so only create one per request. + // UmbracoDatabase is marked IDisposeOnRequestEnd and therefore will be disposed when + // UmbracoModule attempts to dispose the relevant HttpContext items. so we DO dispose + // connections at the end of each request. no need to call ReleaseDatabase. + LogHelper.Debug("Get http [T" + Environment.CurrentManagedThreadId + "]"); + if (HttpContext.Current.Items.Contains(typeof(DefaultDatabaseFactory)) == false) + { + LogHelper.Debug("Create http [T" + Environment.CurrentManagedThreadId + "]"); + HttpContext.Current.Items.Add(typeof(DefaultDatabaseFactory), + string.IsNullOrEmpty(ConnectionString) == false && string.IsNullOrEmpty(ProviderName) == false ? new UmbracoDatabase(ConnectionString, ProviderName, _logger) : new UmbracoDatabase(_connectionStringName, _logger)); - } - return (UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)]; - } + } + return (UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)]; + } - protected override void DisposeResources() + // releases the "context" database + public void ReleaseDatabase() + { + if (HttpContext.Current == null) + { + var value = NonContextValue; + if (value != null) value.Dispose(); + NonContextValue = null; + } + else + { + var db = (UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)]; + if (db != null) db.Dispose(); + HttpContext.Current.Items.Remove(typeof(DefaultDatabaseFactory)); + } + } + + protected override void DisposeResources() { - if (HttpContext.Current == null) - { - _nonHttpInstance.Dispose(); - } - else - { - if (HttpContext.Current.Items.Contains(typeof(DefaultDatabaseFactory))) - { - ((UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)]).Dispose(); - } - } + ReleaseDatabase(); } // during tests, the thread static var can leak between tests // this method provides a way to force-reset the variable internal void ResetForTests() { - if (_nonHttpInstance == null) return; - _nonHttpInstance.Dispose(); - _nonHttpInstance = null; - } - } + var value = NonContextValue; + if (value != null) value.Dispose(); + NonContextValue = null; + } + + #region SafeCallContext + + // see notes in SafeCallContext - need to do this since we are using + // the logical call context... + + static DefaultDatabaseFactory() + { + SafeCallContext.Register(DetachDatabase, AttachDatabase); + } + + // detaches the current database + // ie returns the database and remove it from whatever is "context" + private static UmbracoDatabase DetachDatabase() + { + if (HttpContext.Current == null) + { + var db = NonContextValue; + NonContextValue = null; + return db; + } + else + { + var db = (UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)]; + HttpContext.Current.Items.Remove(typeof(DefaultDatabaseFactory)); + return db; + } + } + + // attach a current database + // ie assign it to whatever is "context" + // throws if there already is a database + private static void AttachDatabase(object o) + { + var database = o as UmbracoDatabase; + if (o != null && database == null) throw new ArgumentException("Not an UmbracoDatabase.", "o"); + + if (HttpContext.Current == null) + { + if (NonContextValue != null) throw new InvalidOperationException(); + if (database != null) NonContextValue = database; + } + else + { + if (HttpContext.Current.Items[typeof(DefaultDatabaseFactory)] != null) throw new InvalidOperationException(); + if (database != null) HttpContext.Current.Items[typeof(DefaultDatabaseFactory)] = database; + } + } + + #endregion + } } \ No newline at end of file diff --git a/src/Umbraco.Core/SafeCallContext.cs b/src/Umbraco.Core/SafeCallContext.cs new file mode 100644 index 0000000000..5ed41d389f --- /dev/null +++ b/src/Umbraco.Core/SafeCallContext.cs @@ -0,0 +1,94 @@ +using System; +using System.Linq; +using System.Collections.Generic; + +namespace Umbraco.Core +{ + internal class SafeCallContext : IDisposable + { + private static readonly List> EnterFuncs = new List>(); + private static readonly List> ExitActions = new List>(); + private static int _count; + private readonly List _objects; + private bool _disposed; + + public static void Register(Func enterFunc, Action exitAction) + { + if (enterFunc == null) throw new ArgumentNullException("enterFunc"); + if (exitAction == null) throw new ArgumentNullException("exitAction"); + + lock (EnterFuncs) + { + if (_count > 0) throw new InvalidOperationException("Cannot register while some SafeCallContext instances exist."); + EnterFuncs.Add(enterFunc); + ExitActions.Add(exitAction); + } + } + + // tried to make the UmbracoDatabase serializable but then it leaks to weird places + // in ReSharper and so on, where Umbraco.Core is not available. Tried to serialize + // as an object instead but then it comes *back* deserialized into the original context + // as an object and of course it breaks everything. Cannot prevent this from flowing, + // and ExecutionContext.SuppressFlow() works for threads but not domains. and we'll + // have the same issue with anything that toys with logical call context... + // + // so this class lets anything that uses the logical call context register itself, + // providing two methods: + // - an enter func that removes and returns whatever is in the logical call context + // - an exit action that restores the value into the logical call context + // whenever a SafeCallContext instance is created, it uses these methods to capture + // and clear the logical call context, and restore it when disposed. + // + // in addition, a static Clear method is provided - which uses the enter funcs to + // remove everything from logical call context - not to be used when the app runs, + // but can be useful during tests + // + // note + // see System.Transactions + // they are using a conditional weak table to store the data, and what they store in + // LLC is the key - which is just an empty MarshalByRefObject that is created with + // the transaction scope - that way, they can "clear current data" provided that + // they have the key - but they need to hold onto a ref to the scope... not ok for us + + public static void Clear() + { + lock (EnterFuncs) + { + foreach (var enter in EnterFuncs) + enter(); + } + } + + public SafeCallContext() + { + lock (EnterFuncs) + { + _count++; + _objects = EnterFuncs.Select(x => x()).ToList(); + } + } + + public void Dispose() + { + if (_disposed) throw new ObjectDisposedException("this"); + _disposed = true; + lock (EnterFuncs) + { + for (var i = 0; i < ExitActions.Count; i++) + ExitActions[i](_objects[i]); + _count--; + } + } + + // for unit tests ONLY + internal static void Reset() + { + lock (EnterFuncs) + { + if (_count > 0) throw new InvalidOperationException("Cannot reset while some SafeCallContext instances exist."); + EnterFuncs.Clear(); + ExitActions.Clear(); + } + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index e180ed7efc..6da7f4519b 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -523,6 +523,7 @@ + diff --git a/src/Umbraco.Tests/Persistence/DatabaseContextTests.cs b/src/Umbraco.Tests/Persistence/DatabaseContextTests.cs index 5919d23acc..1b42daa47c 100644 --- a/src/Umbraco.Tests/Persistence/DatabaseContextTests.cs +++ b/src/Umbraco.Tests/Persistence/DatabaseContextTests.cs @@ -37,7 +37,7 @@ namespace Umbraco.Tests.Persistence { DatabaseContext = _dbContext, IsReady = true - }; + }; } [TearDown] @@ -102,7 +102,7 @@ namespace Umbraco.Tests.Persistence var appCtx = new ApplicationContext( _dbContext, - new ServiceContext(migrationEntryService: Mock.Of()), + new ServiceContext(migrationEntryService: Mock.Of()), CacheHelper.CreateDisabledCacheHelper(), new ProfilingLogger(Mock.Of(), Mock.Of())); diff --git a/src/UmbracoExamine/DataServices/UmbracoContentService.cs b/src/UmbracoExamine/DataServices/UmbracoContentService.cs index 3cd94751ae..8f995a125e 100644 --- a/src/UmbracoExamine/DataServices/UmbracoContentService.cs +++ b/src/UmbracoExamine/DataServices/UmbracoContentService.cs @@ -102,7 +102,6 @@ namespace UmbracoExamine.DataServices { try { - var result = _applicationContext.DatabaseContext.Database.Fetch("select distinct alias from cmsPropertyType order by alias"); return result; }