Reputation: 108
I hope I can summarize this issue well enough. Let's start with the setup.
We have an ASP.NET Core Application and use Entity Framework (without Core). We use a general AuthorizeFilter for all controllers:
services.AddMvc(options =>
{
var policy = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();
options.Filters.Add(new AuthorizeFilter(policy));
})
And we have additional authorize policies applied to some controllers. e.g.:
[ApiController]
[Authorize("UserHasActivatedFeature")]
[Authorize("IsPremiumUser")]
public class PremiumFeatureController : ControllerBase
These policies are handled both with a single IAuthorizationHandler implementation. Looking like this:
public class UserAuthorizationHandler : IAuthorizationHandler
{
private readonly IUserRepository _userRepository;
public UserAuthorizationHandler(IUserRepository userRepository)
{
_userRepository = userRepository;
}
public Task HandleAsync(AuthorizationHandlerContext context)
{
var pendingRequirements = context.PendingRequirements.ToList();
foreach (var requirement in pendingRequirements)
{
switch (requirement)
{
case PremiumRequirement:
if (_userRepository.IsPremium(userId))
{
context.Succeed(requirement);
}
break;
case FeatureActivatedRequirement:
if (_userRepository.IsFeatureActivated(userId))
{
context.Succeed(requirement);
}
break;
}
}
return Task.CompletedTask;
}
}
and the UserRepository looks like this:
public class UserRepository : IRepository<User>, IUserRepository
{
private DbSet<User> DbSet { get; private set; }
public UserRepository(UnitOfWork unitOfWork)
{
DbSet = unitOfWork.DbContext.Set<User>();
}
public bool IsPremium(int userId)
{
return DbSet.AsNoTracking().Any(user => user.Id == userId && user.IsPremium);
}
public bool IsFeatureActivated(int userId)
{
return DbSet.AsNoTracking().Any(user => user.Id == userId && user.IsFeatureActivated);
}
}
On our test environment, where about 20 people are testing our application. There are sometimes exceptions thrown from UserAuthorizationHandler saying: "There is already an open DataReader associated with this Command which must be closed first". When this exception is thrown it is thrown twice once from IsPremium and once from IsFeatureActivated.
System.Data.Entity.Core.EntityCommandExecutionException: An error occurred while executing the command definition. See the inner exception for details. ---> System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first.
at System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command)
at System.Data.SqlClient.SqlCommand.ValidateCommand(String method, Boolean async)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
at System.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior, String method)
at System.Data.Entity.Infrastructure.Interception.InternalDispatcher`1.Dispatch[TTarget,TInterceptionContext,TResult](TTarget target, Func`3 operation, TInterceptionContext interceptionContext, Action`3 executing, Action`3 executed)
at System.Data.Entity.Infrastructure.Interception.DbCommandDispatcher.Reader(DbCommand command, DbCommandInterceptionContext interceptionContext)
at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.ExecuteStoreCommands(EntityCommand entityCommand, CommandBehavior behavior)
--- End of inner exception stack trace ---
at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.ExecuteStoreCommands(EntityCommand entityCommand, CommandBehavior behavior)
at System.Data.Entity.Core.Objects.Internal.ObjectQueryExecutionPlan.Execute[TResultType](ObjectContext context, ObjectParameterCollection parameterValues)
at System.Data.Entity.Core.Objects.ObjectContext.ExecuteInTransaction[T](Func`1 func, IDbExecutionStrategy executionStrategy, Boolean startLocalTransaction, Boolean releaseConnectionOnSuccess)
at System.Data.Entity.Core.Objects.ObjectQuery`1.<>c__DisplayClass41_0.<GetResults>b__0()
at System.Data.Entity.SqlServer.DefaultSqlExecutionStrategy.Execute[TResult](Func`1 operation)
at System.Data.Entity.Core.Objects.ObjectQuery`1.GetResults(Nullable`1 forMergeOption)
at System.Data.Entity.Core.Objects.ObjectQuery`1.<System.Collections.Generic.IEnumerable<T>.GetEnumerator>b__31_0()
at System.Data.Entity.Internal.LazyEnumerator`1.MoveNext()
at System.Linq.Enumerable.Single[TSource](IEnumerable`1 source)
at awesomeCompany.Repositories.UserRepository.IsPremium(Int32 userId)
at awesomeCompany.API.Handlers.UserAuthorizationHandler.HandleAsync(AuthorizationHandlerContext context)
Inner Exception:
System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first.
at System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command)
at System.Data.SqlClient.SqlCommand.ValidateCommand(String method, Boolean async)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
at System.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior, String method)
at System.Data.Entity.Infrastructure.Interception.InternalDispatcher`1.Dispatch[TTarget,TInterceptionContext,TResult](TTarget target, Func`3 operation, TInterceptionContext interceptionContext, Action`3 executing, Action`3 executed)
at System.Data.Entity.Infrastructure.Interception.DbCommandDispatcher.Reader(DbCommand command, DbCommandInterceptionContext interceptionContext)
at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.ExecuteStoreCommands(EntityCommand entityCommand, CommandBehavior behavior)
As I said above this exception only appears from time to time (like 2 or 3 times a day) so I tried to isolate the issue locally and created unit tests to test the repository with multiple parallel calls at once, but I could not reproduce the issue. Calling the methods 100 times at the same time seams to be completely fine.
I read many similar questions here on stackoverflow. Many seam to have this issue when using a query inside another query or inside a IEnumerable loop, but since I am only using an Any() method on the database entities inside a normal foreach-loop iterating through a list that is completely unrelated from the userRepository I am not sure if any of the solutions apply here.
Also applying the Setting MultipleActiveResultSets=true to the connection string seams not to be a good solution and I would only consider it as a last option.
Upvotes: 0
Views: 2908
Reputation: 131180
The problem is using a singleton in the first place. A DbContext is neither thread-safe nor meant to be long lived. It's a Unit-of-Work that caches any pending changes and either commits them all atomically when SaveChanges
is called, or discards them when disposed. This means a DbContext should always be disposed once the unit of work is done.
Nothing is gained by using a singleton DbContext either. It's not a connection, so no time is saved by keeping it around for long. A DbContext opens and closes connections itself when it has to read data or persist changes. It doesn't keep them alive.
By making IAuthorizationHandler
you're also keeping a DbContext
instance as a singleton and end up using it concurrently from multiple requests. You may not get any exceptions until two requests try to read from the DbContext instance simultaneously.
Solution
Make UserAuthorizationHandler scoped
To fix this problem, the easiest solution seems to be to make UserAuthorizationHandler
a scoped instance. The code posted here doesn't seem to use any other singleton services so it doesn't need to be a singleton itself.
Use scopes explicitly
If it has to remain a singleton, the DbContext and by extension, the UserRepository will have to be created inside a scope as needed. In this case, inside HandleAsync
. To do that you need to inject IServiceProvider
instead of IUserRepository
. This is explained in Consuming a scoped service in a background task :
public class UserAuthorizationHandler : IAuthorizationHandler
{
private readonly IServiceProvider _services;
public UserAuthorizationHandler(IServiceProvider services)
{
_services = services;
}
public Task HandleAsync(AuthorizationHandlerContext context)
{
using (var scope = Services.CreateScope())
{
using(var userRepository=scope.GetRequiredService<IUserRepository>())
{
var pendingRequirements = context.PendingRequirements.ToList();
...
}
}
}
BTW don't use blocking IO/database calls in an async method. This blocks a thread that could be used to serve other requests and increasing CPU usage due to spinwaits. A lot of the performance gains in ASP.NET Core come from avoiding unnecessary blocks
This means that either IsPremium
and IsFeatureActivated
should become asynchronous, or, if the synchronous methods are really needed, that async versions should be added. In this particular case both methods are used, so a good idea would be to use a single query to return both values :
public async Task<(bool isPremium,isFeatureActivated)> FeaturesForAsync(int userId)
{
var results=await _context.Users
.Where(u=>u.Id==userId)
.Select(u=>new {u.IsPremium,u.IsFeatureActivated})
.SingleOrDefaultAsync();
if (results==null) return default;
return (results.IsPremium,results.IsFeatureActivated);
}
Or, using records instead of tuples :
record UserFeatures(bool IsPremium,bool IsFeatureActivated);
public async Task<(bool isPremium,isFeatureActivated)> FeaturesForAsync(int userId)
{
var results=await _context.Users
.Where(u=>u.Id==userId)
.Select(u=>new UserFeatures(
u.IsPremium,
u.IsFeatureActivated))
.SingleOrDefaultAsync();
return results ?? new UserFeatures(false,false);
}
Upvotes: 2