From 2258eeae3908b4729dbbf2d5b95974986fb1ec3a Mon Sep 17 00:00:00 2001 From: Andrew Peters Date: Tue, 30 May 2017 14:52:44 -0700 Subject: [PATCH] Database Error Page: Remove ILogger-based interception. - Made EF calls async - Removed a slow, repeated DB existence check. --- .../DataStoreErrorLogger.cs | 98 ---------- .../DataStoreErrorLoggerProvider.cs | 27 --- .../DatabaseErrorPageMiddleware.cs | 181 ++++++++++++------ .../DatabaseErrorPageOptions.cs | 1 + .../MigrationsEndPointExtensions.cs | 1 + .../MigrationsEndPointOptions.cs | 1 + .../breakingchanges.netcore.json | 17 ++ 7 files changed, 140 insertions(+), 186 deletions(-) delete mode 100644 src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLogger.cs delete mode 100644 src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLoggerProvider.cs diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLogger.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLogger.cs deleted file mode 100644 index f0db3de1eb..0000000000 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLogger.cs +++ /dev/null @@ -1,98 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using JetBrains.Annotations; -using Microsoft.EntityFrameworkCore; -using Microsoft.EntityFrameworkCore.Diagnostics; -using Microsoft.EntityFrameworkCore.Infrastructure; -using Microsoft.EntityFrameworkCore.Storage; -using Microsoft.Extensions.Logging; -using System; -using System.Collections.Generic; -using System.Linq; -using System.Threading; - -namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore -{ - public class DataStoreErrorLogger : ILogger - { - private static readonly AsyncLocal _log = new AsyncLocal(); - - public virtual DataStoreErrorLog LastError - { - get - { - return _log.Value; - } - } - - public virtual void StartLoggingForCurrentCallContext() - { - // Because CallContext is cloned at each async operation we cannot - // lazily create the error object when an error is encountered, otherwise - // it will not be available to code outside of the current async context. - // We create it ahead of time so that any cloning just clones the reference - // to the object that will hold any errors. - _log.Value = new DataStoreErrorLog(); - } - - public virtual void Log( - LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) - { - if (exception != null - && LastError != null - && (eventId.Id == CoreEventId.SaveChangesFailed.Id - || eventId.Id == CoreEventId.QueryIterationFailed.Id)) - { - LastError.SetError( - (Type)((IReadOnlyList>)state).Single(p => p.Key == "contextType").Value, - exception); - } - } - - public virtual bool IsEnabled(LogLevel logLevel) - { - return true; - } - - public virtual IDisposable BeginScope(TState state) - { - return NullScope.Instance; - } - - private class NullScope : IDisposable - { - public static NullScope Instance = new NullScope(); - - public void Dispose() - { } - } - - public class DataStoreErrorLog - { - private Type _contextType; - private Exception _exception; - - public virtual void SetError(Type contextType, Exception exception) - { - _contextType = contextType; - _exception = exception; - } - - public virtual bool IsErrorLogged - { - get { return _exception != null; } - } - - public virtual Type ContextType - { - get { return _contextType; } - } - - public virtual Exception Exception - { - get { return _exception; } - } - } - } -} diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLoggerProvider.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLoggerProvider.cs deleted file mode 100644 index 3af3d6ecac..0000000000 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DataStoreErrorLoggerProvider.cs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using Microsoft.Extensions.Logging; - -namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore -{ - public class DataStoreErrorLoggerProvider : ILoggerProvider - { - private readonly DataStoreErrorLogger _logger = new DataStoreErrorLogger(); - - public virtual ILogger CreateLogger(string name) - { - return _logger; - } - - public virtual DataStoreErrorLogger Logger - { - get { return _logger; } - } - - public virtual void Dispose() - { - } - } -} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageMiddleware.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageMiddleware.cs index 57d0b73823..043bbc1b60 100644 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageMiddleware.cs +++ b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageMiddleware.cs @@ -2,12 +2,16 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; +using System.Diagnostics; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.Views; using Microsoft.AspNetCore.Http; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.EntityFrameworkCore.Storage; @@ -17,23 +21,36 @@ using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore { /// - /// Captures synchronous and asynchronous database related exceptions from the pipeline that may be resolved using Entity Framework - /// migrations. When these exceptions occur an HTML response with details of possible actions to resolve the issue is generated. + /// Captures synchronous and asynchronous database related exceptions from the pipeline that may be resolved using Entity Framework + /// migrations. When these exceptions occur an HTML response with details of possible actions to resolve the issue is generated. /// - public class DatabaseErrorPageMiddleware + public class DatabaseErrorPageMiddleware : IObserver, IObserver> { + private static readonly AsyncLocal _localDiagnostic = new AsyncLocal(); + + private sealed class DiagnosticHolder + { + public void Hold(Exception exception, Type contextType) + { + Exception = exception; + ContextType = contextType; + } + + public Exception Exception { get; private set; } + public Type ContextType { get; private set; } + } + private readonly RequestDelegate _next; private readonly DatabaseErrorPageOptions _options; private readonly ILogger _logger; - private readonly DataStoreErrorLoggerProvider _loggerProvider; /// - /// Initializes a new instance of the class + /// Initializes a new instance of the class /// /// Delegate to execute the next piece of middleware in the request pipeline. /// - /// The for the application. This middleware both produces logging messages and - /// consumes them to detect database related exception. + /// The for the application. This middleware both produces logging messages and + /// consumes them to detect database related exception. /// /// The options to control what information is displayed on the error page. public DatabaseErrorPageMiddleware( @@ -60,90 +77,84 @@ namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore _options = options.Value; _logger = loggerFactory.CreateLogger(); - _loggerProvider = new DataStoreErrorLoggerProvider(); -#pragma warning disable CS0618 // Type or member is obsolete - loggerFactory.AddProvider(_loggerProvider); -#pragma warning restore CS0618 // Type or member is obsolete + DiagnosticListener.AllListeners.Subscribe(this); } /// - /// Process an individual request. + /// Process an individual request. /// - /// The context for the current request. + /// The HTTP context for the current request. /// A task that represents the asynchronous operation. - public virtual async Task Invoke(HttpContext context) + public virtual async Task Invoke(HttpContext httpContext) { - if (context == null) + if (httpContext == null) { - throw new ArgumentNullException(nameof(context)); + throw new ArgumentNullException(nameof(httpContext)); } try { - _loggerProvider.Logger.StartLoggingForCurrentCallContext(); + // Because CallContext is cloned at each async operation we cannot + // lazily create the error object when an error is encountered, otherwise + // it will not be available to code outside of the current async context. + // We create it ahead of time so that any cloning just clones the reference + // to the object that will hold any errors. - await _next(context); + _localDiagnostic.Value = new DiagnosticHolder(); + + await _next(httpContext); } - catch (Exception ex) + catch (Exception exception) { try { - if (ShouldDisplayErrorPage(_loggerProvider.Logger.LastError, ex, _logger)) + if (ShouldDisplayErrorPage(exception)) { - var dbContextType = _loggerProvider.Logger.LastError.ContextType; - var dbContext = (DbContext) context.RequestServices.GetService(dbContextType); + var contextType = _localDiagnostic.Value.ContextType; + var context = (DbContext)httpContext.RequestServices.GetService(contextType); - if (dbContext == null) + if (context == null) { - _logger.LogError( - Strings.FormatDatabaseErrorPageMiddleware_ContextNotRegistered(dbContextType.FullName)); + _logger.LogError(Strings.FormatDatabaseErrorPageMiddleware_ContextNotRegistered(contextType.FullName)); } else { - var creator = dbContext.GetService() as IRelationalDatabaseCreator; + var relationalDatabaseCreator = context.GetService() as IRelationalDatabaseCreator; - if (creator == null) + if (relationalDatabaseCreator == null) { _logger.LogDebug(Strings.DatabaseErrorPage_NotRelationalDatabase); } else { - var databaseExists = creator.Exists(); + var databaseExists = await relationalDatabaseCreator.ExistsAsync(); - var historyRepository = dbContext.GetService(); - var migrationsAssembly = dbContext.GetService(); - var modelDiffer = dbContext.GetService(); - - var appliedMigrations = historyRepository.GetAppliedMigrations(); - - var pendingMigrations - = (from m in migrationsAssembly.Migrations - where !appliedMigrations.Any( - r => string.Equals(r.MigrationId, m.Key, - StringComparison.OrdinalIgnoreCase)) - select m.Key) - .ToList(); + var migrationsAssembly = context.GetService(); + var modelDiffer = context.GetService(); // HasDifferences will return true if there is no model snapshot, but if there is an existing database // and no model snapshot then we don't want to show the error page since they are most likely targeting // and existing database and have just misconfigured their model - var pendingModelChanges - = (migrationsAssembly.ModelSnapshot != null || !databaseExists) - && modelDiffer.HasDifferences(migrationsAssembly.ModelSnapshot?.Model, - dbContext.Model); - if (!databaseExists && pendingMigrations.Any() - || pendingMigrations.Any() - || pendingModelChanges) + var pendingModelChanges + = (!databaseExists || migrationsAssembly.ModelSnapshot != null) + && modelDiffer.HasDifferences(migrationsAssembly.ModelSnapshot?.Model, context.Model); + + var pendingMigrations + = (databaseExists + ? await context.Database.GetPendingMigrationsAsync() + : context.Database.GetMigrations()) + .ToArray(); + + if (pendingModelChanges || pendingMigrations.Any()) { var page = new DatabaseErrorPage { Model = new DatabaseErrorPageModel( - dbContextType, ex, databaseExists, pendingModelChanges, pendingMigrations, - _options) + contextType, exception, databaseExists, pendingModelChanges, pendingMigrations, _options) }; - await page.ExecuteAsync(context); + await page.ExecuteAsync(httpContext); return; } @@ -153,21 +164,26 @@ namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore } catch (Exception e) { - _logger.LogError(0, e, Strings.DatabaseErrorPageMiddleware_Exception); + _logger.LogError( + eventId: 0, + exception: e, + message: Strings.DatabaseErrorPageMiddleware_Exception); } throw; } } - private static bool ShouldDisplayErrorPage(DataStoreErrorLogger.DataStoreErrorLog lastError, - Exception exception, ILogger logger) + private bool ShouldDisplayErrorPage(Exception exception) { - logger.LogDebug(Strings.FormatDatabaseErrorPage_AttemptingToMatchException(exception.GetType())); + _logger.LogDebug(Strings.FormatDatabaseErrorPage_AttemptingToMatchException(exception.GetType())); - if (!lastError.IsErrorLogged) + var lastRecordedException = _localDiagnostic.Value.Exception; + + if (lastRecordedException == null) { - logger.LogDebug(Strings.DatabaseErrorPage_NoRecordedException); + _logger.LogDebug(Strings.DatabaseErrorPage_NoRecordedException); + return false; } @@ -175,19 +191,62 @@ namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore for (var e = exception; e != null && !match; e = e.InnerException) { - match = lastError.Exception == e; + match = lastRecordedException == e; } if (!match) { - logger.LogDebug(Strings.DatabaseErrorPage_NoMatch); + _logger.LogDebug(Strings.DatabaseErrorPage_NoMatch); return false; } - logger.LogDebug(Strings.DatabaseErrorPage_Matched); + _logger.LogDebug(Strings.DatabaseErrorPage_Matched); return true; } + + void IObserver.OnNext(DiagnosticListener diagnosticListener) + { + if (diagnosticListener.Name == DbLoggerCategory.Root) + { + diagnosticListener.Subscribe(this); + } + } + + void IObserver>.OnNext(KeyValuePair keyValuePair) + { + switch (keyValuePair.Value) + { + case DbContextErrorEventData contextErrorEventData: + { + _localDiagnostic.Value.Hold(contextErrorEventData.Exception, contextErrorEventData.Context.GetType()); + + break; + } + case DbContextTypeErrorEventData contextTypeErrorEventData: + { + _localDiagnostic.Value.Hold(contextTypeErrorEventData.Exception, contextTypeErrorEventData.ContextType); + + break; + } + } + } + + void IObserver.OnCompleted() + { + } + + void IObserver.OnError(Exception error) + { + } + + void IObserver>.OnCompleted() + { + } + + void IObserver>.OnError(Exception error) + { + } } -} \ No newline at end of file +} diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageOptions.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageOptions.cs index 38d89e2284..765467e5f6 100644 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageOptions.cs +++ b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/DatabaseErrorPageOptions.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore; using Microsoft.AspNetCore.Http; +// ReSharper disable once CheckNamespace namespace Microsoft.AspNetCore.Builder { /// diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointExtensions.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointExtensions.cs index 9f688126a2..c635bb43fd 100644 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointExtensions.cs +++ b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointExtensions.cs @@ -5,6 +5,7 @@ using System; using Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore; using Microsoft.Extensions.Options; +// ReSharper disable once CheckNamespace namespace Microsoft.AspNetCore.Builder { /// diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointOptions.cs b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointOptions.cs index a26e897993..48b053c13f 100644 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointOptions.cs +++ b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/MigrationsEndPointOptions.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore; using Microsoft.AspNetCore.Http; +// ReSharper disable once CheckNamespace namespace Microsoft.AspNetCore.Builder { /// diff --git a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/breakingchanges.netcore.json b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/breakingchanges.netcore.json index d8cfddf0aa..bf916c182f 100644 --- a/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/breakingchanges.netcore.json +++ b/src/Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore/breakingchanges.netcore.json @@ -16,5 +16,22 @@ "TypeId": "public class Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware", "MemberId": "public .ctor(Microsoft.AspNetCore.Http.RequestDelegate next, System.IServiceProvider serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.Extensions.Options.IOptions options)", "Kind": "Removal" + }, + { + "TypeId": "public class Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DataStoreErrorLogger : Microsoft.Extensions.Logging.ILogger", + "Kind": "Removal" + }, + { + "TypeId": "public class Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DataStoreErrorLogger+DataStoreErrorLog", + "Kind": "Removal" + }, + { + "TypeId": "public class Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DataStoreErrorLoggerProvider : Microsoft.Extensions.Logging.ILoggerProvider", + "Kind": "Removal" + }, + { + "TypeId": "public class Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware", + "MemberId": "public virtual System.Threading.Tasks.Task Invoke(Microsoft.AspNetCore.Http.HttpContext context)", + "Kind": "Removal" } ] \ No newline at end of file